Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Calyx] Lower Arith CmpFOp to Calyx #7860

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jiahanxie353
Copy link
Contributor

This patch tries to lowers arith::cmpf to Calyx. Relevant issue: #7821

One thing I'm still trying to figure out is that, taking ult as an example, I tried to create a CombGroup to evaluate std_compareFN.lt || std_compareFN.unordered.

My test case is:

module {
  func.func @main(%arg0 : f32) -> i1 {
    %0 = arith.constant 4.2 : f32
    %1 = arith.cmpf ult, %arg0, %0 : f32

    return %1 : i1
  }
}

and I dump intermediate result after BuildOpGroups, it shows:

"calyx.component"() <{function_type = (i32, i1, i1, i1, i1, i1) -> (), portAttributes = [{}, {clk}, {reset}, {go}, {}, {done}], portDirections = -16 : i6, portNames = ["in0", "clk", "reset", "go", "out0", "done"]}> ({
^bb0(%arg0: i32, %arg1: i1, %arg2: i1, %arg3: i1, %arg4: i1, %arg5: i1):
  %0 = "hw.constant"() <{value = true}> : () -> i1
  %1 = "hw.constant"() <{value = true}> : () -> i1
  %2:6 = "calyx.register"() <{sym_name = "cmpf_0_reg"}> : () -> (i1, i1, i1, i1, i1, i1)
  %3:3 = "calyx.std_or"() <{sym_name = "std_or_0"}> : () -> (i1, i1, i1)
  %4 = "hw.constant"() <{value = true}> : () -> i1
  %5:12 = "calyx.ieee754.compare"() <{sym_name = "std_compareFN_0"}> : () -> (i1, i1, i1, i32, i32, i1, i1, i1, i1, i1, i5, i1)
  %6:6 = "calyx.register"() <{sym_name = "ret_arg0_reg"}> : () -> (i1, i1, i1, i1, i1, i1)
  %7 = "calyx.constant"() <{sym_name = "cst_0", value = 4.200000e+00 : f32}> : () -> i32
  "calyx.wires"() ({
    "calyx.assign"(%arg4, %6#4) : (i1, i1) -> ()
    "calyx.comb_group"() <{sym_name = "compute_compare_0_out"}> ({
      "calyx.assign"(%3#0, %5#9) : (i1, i1) -> ()
      "calyx.assign"(%3#1, %5#6) : (i1, i1) -> ()
    }) : () -> ()
    "calyx.group"() <{sym_name = "bb0_0"}> ({
      "calyx.assign"(%5#3, %arg0) : (i32, i32) -> ()
      "calyx.assign"(%5#4, %7) : (i32, i32) -> ()
      "calyx.assign"(%2#0, %3#2) : (i1, i1) -> ()
      "calyx.assign"(%2#1, %5#11) : (i1, i1) -> ()
      %8 = "hw.constant"() <{value = true}> : () -> i1
      %9 = "comb.xor"(%5#11, %8) : (i1, i1) -> i1
      "calyx.assign"(%5#2, %1, %9) : (i1, i1, i1) -> ()
      "calyx.group_done"(%2#5) : (i1) -> ()
    }) : () -> ()
    "calyx.group"() <{sym_name = "ret_assign_0"}> ({
      "calyx.assign"(%6#0, %3#2) : (i1, i1) -> ()
      "calyx.assign"(%6#1, %0) : (i1, i1) -> ()
      "calyx.group_done"(%6#5) : (i1) -> ()
    }) : () -> ()
  }) : () -> ()
  "calyx.control"() ({
  ^bb0:
  }) : () -> ()
}) {sym_name = "main", toplevel} : () -> ()

so clearly we can see:

    "calyx.comb_group"() <{sym_name = "compute_compare_0_out"}> ({
      "calyx.assign"(%3#0, %5#9) : (i1, i1) -> ()
      "calyx.assign"(%3#1, %5#6) : (i1, i1) -> ()
    }) : () -> ()

But after the entire execution, it finishes with:

module attributes {calyx.entrypoint = "main"} {
  calyx.component @main(%in0: i32, %clk: i1 {clk}, %reset: i1 {reset}, %go: i1 {go}) -> (%out0: i1, %done: i1 {done}) {
    %cst = calyx.constant @cst_0 <4.200000e+00 : f32> : i32
    %true = hw.constant true
    %cmpf_0_reg.in, %cmpf_0_reg.write_en, %cmpf_0_reg.clk, %cmpf_0_reg.reset, %cmpf_0_reg.out, %cmpf_0_reg.done = calyx.register @cmpf_0_reg : i1, i1, i1, i1, i1, i1
    %std_or_0.left, %std_or_0.right, %std_or_0.out = calyx.std_or @std_or_0 : i1, i1, i1
    %std_compareFN_0.clk, %std_compareFN_0.reset, %std_compareFN_0.go, %std_compareFN_0.left, %std_compareFN_0.right, %std_compareFN_0.signaling, %std_compareFN_0.lt, %std_compareFN_0.eq, %std_compareFN_0.gt, %std_compareFN_0.unordered, %std_compareFN_0.exceptionalFlags, %std_compareFN_0.done = calyx.ieee754.compare @std_compareFN_0 : i1, i1, i1, i32, i32, i1, i1, i1, i1, i1, i5, i1
    %ret_arg0_reg.in, %ret_arg0_reg.write_en, %ret_arg0_reg.clk, %ret_arg0_reg.reset, %ret_arg0_reg.out, %ret_arg0_reg.done = calyx.register @ret_arg0_reg : i1, i1, i1, i1, i1, i1
    calyx.wires {
      calyx.assign %out0 = %ret_arg0_reg.out : i1
      calyx.group @bb0_0 {
        calyx.assign %std_compareFN_0.left = %in0 : i32
        calyx.assign %std_compareFN_0.right = %cst : i32
        calyx.assign %cmpf_0_reg.in = %std_or_0.out : i1
        calyx.assign %cmpf_0_reg.write_en = %std_compareFN_0.done : i1
        %0 = comb.xor %std_compareFN_0.done, %true : i1
        calyx.assign %std_compareFN_0.go = %0 ? %true : i1
        calyx.group_done %cmpf_0_reg.done : i1
      }
      calyx.group @ret_assign_0 {
        calyx.assign %ret_arg0_reg.in = %std_or_0.out : i1
        calyx.assign %ret_arg0_reg.write_en = %true : i1
        calyx.group_done %ret_arg0_reg.done : i1
      }
    }
    calyx.control {
      calyx.seq {
          calyx.enable @bb0_0
          calyx.enable @ret_assign_0
      }
    }
  } {toplevel}
}

There's no appearance of that combinational group. Any insight why could this happen? @cgyurgyik @andrewb1999 @rachitnigam

@cgyurgyik
Copy link
Member

My first guess is this is being inlined because of InlineCombGroups.

@jiahanxie353
Copy link
Contributor Author

My first guess is this is being inlined because of InlineCombGroups.

Makes sense, and I think I shouldn’t put them into comb groups in the first place since std_compareFN.lt/unordered are sequential. And does that mean I should make registers to hold std_compareFN.lt/unordered values?

@cgyurgyik
Copy link
Member

Makes sense, and I think I shouldn’t put them into comb groups in the first place since std_compareFN.lt/unordered are sequential. And does that mean I should make registers to hold std_compareFN.lt/unordered values?

Yes, if you want these to be read in sequential order then they need to be placed in registers.

}

// General case
StringRef opName = cmpf.getOperationName().split(".").second;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should refactor with buildLibraryBinaryPipeOp, it's tricky though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants