Support scf.if Op Lowering to Calyx

[jiahanxie353]

This PR tries to add support for scf.if during lowering to Calyx, as mentioned in this issue #4843

Currently this PR is still in early stages and contains some issues, but I believe that some guidance would help ensure I'm on the right track!

My Thoughts and Progress

To add support for scf.if, I think we need:

• The structure for if op lowering;
• Basic block analysis fir Then/Else region;
• Proper handling block arguments and yield values

Challenges/Issues

• I'm not sure if my overall approach is on the right track in general, I'd appreciate any insights or suggestions!
• I'm trying to debug/step in the code I'm writing but somehow this newly created goupOp is empty and only has void in the body, as in this line, so I couldn't move on to debug other issues (but ofc this approach could be wrong in the first place).

I understand that this is still a work-in-progress, and there are several rough edges. I'm hoping to get some guidance on the general direction and any potential pitfalls I might encounter. Any feedback, suggestions, or advice would be greatly appreciated!

[cgyurgyik]

It would be helpful to have some minimal code examples of what you expect and what you want.

[cgyurgyik]

You'll want to make sure there are no name clashes. See, e.g.,

circt/lib/Conversion/CalyxToFSM/CalyxToFSM.cpp

Lines 305 to 310 in 307fb58

	// The following loop is used to check if the transitionName already exists.
	// If it does, the loop regenerates the transitionName.
	do {
	transitionName = transitionNameHead + std::to_string(transitionNameTail++);
	} while (component.getWiresOp().lookupSymbol(transitionName));
	return transitionName;

[mikeurbach]

I think this is a good start, thanks for working on this!

I'm not sure why you are seeing the GroupOp body is void. The GroupOp builder, which is called by createGroup, should create a block, and buildAssignmentsForRegisterWrite should add assign ops to that block.

[mikeurbach]

I don't think this is used, is it intended to be used? I think in general, we probably don't need the interfaces you've added here, if there is only one thing implementing the interface. The reason we have interfaces for the while op is because we want to generically handle both scf::WhileOp and our own loopschedule::WhileOp.

[jiahanxie353]

I think you are right, adding an interface is redundant for if

[jiahanxie353]

It would be helpful to have some minimal code examples of what you expect and what you want.

Sure! I'm working on to make the example in the mentioned issue working:

func.func @main(%arg0 : i32, %arg1 : i32) -> i32 {
  %0 = arith.cmpi slt, %arg0, %arg1 : i32
  %1 = scf.if %0 -> i32 {
    %3 = arith.addi %arg0, %arg1 : i32
    scf.yield %3 : i32
  } else {
    scf.yield %arg1 : i32
  }
}

in which I try prepare the registers and to yield different results based on the value of %0:
If %0 == true, I will enter the then region, create a calyx group, and create necessary registers. And I'd expect to see %3 is yielded.
And same for the else region and expect to see %arg1 yielded

[jiahanxie353]

Hi all, I thought it'd be nice if I take a step back and first decide what's my expected lowered IR would look like.
Assume we are using the same example:

func.func @main(%arg0 : i32, %arg1 : i32) -> i32 {
  %0 = arith.cmpi slt, %arg0, %arg1 : i32
  %1 = scf.if %0 -> i32 {
    %2 = arith.addi %arg0, %arg1 : i32
    scf.yield %2 : i32
  } else {
    scf.yield %arg1 : i32
  }
  return %1 : i32
}

After some research, I think my desired IR after lowering would be something like:

module attributes {calyx.entrypoint = "main"} {
calyx.component @main(%in0: i32, %in1: i32, %clk: i1 {clk}, %reset: i1 {reset}, %go: i1 {go}) -> (%out0: i32, %done: i1 {done}) {
  %std_slt_0.left, %std_slt_0.right, %std_slt_0.out = calyx.std_slt @std_slt_0 : i32, i32, i1
  %std_add_0.left, %std_add_0.right, %std_add_0.out = calyx.std_add @std_add_0 : i32, i32, i32
  %yield_reg.in, %yield_reg.write_en, %yield_reg.clk, %yield_reg.reset, %yield_reg.out, %yield_reg.done = calyx.register @yield_reg : i32, i1, i1, i1, i32, i1
  %ret_reg.in, %ret_reg.write_en, %ret_reg.clk, %ret_reg.reset, %ret_reg.out, %ret_reg.done = calyx.register @ret_reg : i32, i1, i1, i1, i32, i1
  calyx.wires {
      calyx.assign %out = %ret_reg.out : i32
      calyx.comb_group @bb0_0 {
            calyx.assign %std_slt.left = %arg0 : i32
            calyx.assign %std_slt.right = %arg1 : i32
      }
      calyx.comb_group @bb0_1 {
            calyx.assign %std_add.left = %arg0 : i32
            calyx.assign %std_add.right = %arg1 : i32
      }
      calyx.group @assign_then_block {
            calyx.assign %yield_reg.in = %std_add.out : i32
            calyx.assign %yield_reg.write_en = %true : i1
            calyx.group_done %yield_reg.done : i1
      }
      calyx.group @assign_else_block {
            calyx.assign %yield_reg.in = %arg1 : i32
            calyx.assign %yield_reg.write_en = %true : i1
            calyx.group_done %yield_reg.done : i1
      }
     calyx.group @ret_assign {
            calyx.assign %ret_reg.in = %yield_reg.out : i32
            calyx.assign %ret_reg.write_en = %true : i1
            calyx.group_done %ret_reg.done : i1
      }
  }
   calyx.control {
     calyx.seq {
       calyx.if %std_slt.out with @bb0_1 {
         calyx.seq {
           calyx.enable @assign_then_block
         }
       }
      calyx.else {
           calyx.enable @assign_else_block
      }
      }
    }
  } {toplevel}
}
}

A few things I'm confused/not confident about are:

• What should we do with the conditional operand of scf.if Should we initialize any Calyx component for it; or should we just wire it?
• I'm not confident about the design choice of creating a yield_reg register for the result of yield because we are not storing states but is rather combinational, except that we could have two possible combinational results based on whether we execute the then branch or the else branch.
• And I'm not confident about the output Calyx control part.

Any insight will be greatly appreciated! :)

[cgyurgyik]

I think the expected IR looks good. You'll want to actually use the ret_assign in your control flow (hopefully the verifier will complain if you didn't).

What should we do with the conditional operand of scf.if Should we initialize any Calyx component for it; or should we just wire it?

Can you elaborate? Do you have an example where just using Calyx control wouldn't work?

I'm not confident about the design choice of creating a yield_reg register for the result of yield because we are not storing states but is rather combinational, except that we could have two possible combinational results based on whether we execute the then branch or the else branch.

IMO, I think a good first step is just creating a yield register and getting that to work. Then, you can discuss more optimal design decisions, e.g., maybe you'd want to canonicalize such simple use cases with a std_mux.

[rachitnigam]

Agreed with @cgyurgyik! I think we should try getting something working first and worry about optimizing the encoding later on. @jiahanxie353 answers to your questions:

What should we do with the conditional operand of scf.if Should we initialize any Calyx component for it; or should we just wire it?

The way scf encodes the conditional, it seems you can store the value of %0 in a register and just use that without the with statement:

if 0_out.out { ... }

I'm not confident about the design choice of creating a yield_reg register for the result of yield because we are not storing states but is rather combinational, except that we could have two possible combinational results based on whether we execute the then branch or the else branch.

This encoding looks good to me! Calyx's control program will ensure that only one of the two groups will ever execute at a time.

And I'm not confident about the output Calyx control part.

I don't see any problems with it!

[jiahanxie353]

Thanks for all the advice @cgyurgyik @rachitnigam !
I was able to make some progress:

1. I have built a register for if's condition operand and assign the value to it;
2. Started to build ifGroup and elseGroup, and I'm trying to assign values inside the ifGroup but I got stuck. As we discussed, I expected to see calyx.assign inside the group:

calyx.group @assign_then_block {
            calyx.assign %yield_reg.in = %std_add.out : i32
            calyx.assign %yield_reg.write_en = %true : i1
            calyx.group_done %yield_reg.done : i1
      }

but now the group's body is empty.
My guess is that I created thenGroup based on ifOp's thenRegion's location, but I'm inserting to ifOp's thenBlock, these two might not reference to the same thing. But the issue I'm having is that I can't getLoc() of a Block but only of a Region or Operation. How can I make two things referencing the same location so that I can insert into it?

Thanks!

[cgyurgyik]

First, I think the Loc you're referring to is related to debugging information, i.e., source-locations.

So you're creating assignment operations and they're not appearing inside the block's body? Where are they appearing?

What you probably want to do is set the insertion point correctly, e.g.,

Block* block = foo.getBody();
rewriter.setInsertionPointToStart(block);

https://mlir.llvm.org/doxygen/classmlir_1_1OpBuilder.html

Example in this project:
https://github.com/llvm/circt/blob/cde4642799450170a06f340f2332eecb60e7b34d/lib/Conversion/SCFToCalyx/SCFToCalyx.cpp#L573C12-L574

[jiahanxie353]

Progress

First, I think the Loc you're referring to is related to debugging information, i.e., source-locations.

Thanks for pointing out, I was confused by these two.

So you're creating assignment operations and they're not appearing inside the block's body? Where are they appearing?

Thanks to @cgyurgyik advice, they are now appearing in the correct position! Shown below:

// -----// IR Dump After SCFToCalyx Failed (lower-scf-to-calyx) //----- //
"builtin.module"() ({
  "calyx.component"() ({
  ^bb0(%arg0: i32, %arg1: i32, %arg2: i1, %arg3: i1, %arg4: i1, %arg5: i32, %arg6: i1):
    %0 = "hw.constant"() {value = true} : () -> i1
    %1:6 = "calyx.register"() {sym_name = "cond_arg_reg"} : () -> (i1, i1, i1, i1, i1, i1)
    %2:6 = "calyx.register"() {sym_name = "else_yield_reg"} : () -> (i32, i1, i1, i1, i32, i1)
    %3:6 = "calyx.register"() {sym_name = "then_yield_reg"} : () -> (i32, i1, i1, i1, i32, i1)
    %4:3 = "calyx.std_add"() {sym_name = "std_add_0"} : () -> (i32, i32, i32)
    %5:3 = "calyx.std_slt"() {sym_name = "std_slt_0"} : () -> (i32, i32, i1)
    %6:6 = "calyx.register"() {sym_name = "ret_arg0_reg"} : () -> (i32, i1, i1, i1, i32, i1)
    "calyx.wires"() ({
      "calyx.assign"(%1#0, %5#2) : (i1, i1) -> ()
      "calyx.assign"(%arg5, %6#4) : (i32, i32) -> ()
      "calyx.comb_group"() ({
        "calyx.assign"(%5#0, %arg0) : (i32, i32) -> ()
        "calyx.assign"(%5#1, %arg1) : (i32, i32) -> ()
      }) {sym_name = "bb0_0"} : () -> ()
      "calyx.comb_group"() ({
        "calyx.assign"(%4#0, %arg0) : (i32, i32) -> ()
        "calyx.assign"(%4#1, %arg1) : (i32, i32) -> ()
      }) {sym_name = "bb0_1"} : () -> ()
      "calyx.group"() ({
        "calyx.assign"(%3#0, %4#0) : (i32, i32) -> ()
        "calyx.assign"(%3#1, %0) : (i1, i1) -> ()
        "calyx.group_done"(%3#5) : (i1) -> ()
      }) {sym_name = "assign_then_group"} : () -> ()
      "calyx.group"() ({
        "calyx.assign"(%2#0, %arg1) : (i32, i32) -> ()
        "calyx.assign"(%2#1, %0) : (i1, i1) -> ()
        "calyx.group_done"(%2#5) : (i1) -> ()
      }) {sym_name = "assign_else_group"} : () -> ()
    }) : () -> ()
    "calyx.control"() ({
    ^bb0:
    }) : () -> ()
  }) {function_type = (i32, i32, i1, i1, i1, i32, i1) -> (), portAttributes = [{}, {}, {clk}, {reset}, {go}, {}, {done}], portDirections = -32 : i7, portNames = ["in0", "in1", "clk", "reset", "go", "out0", "done"], sym_name = "main", toplevel} : () -> ()
  "func.func"() <{function_type = (i32, i32) -> i32, sym_name = "func_main"}> ({
  ^bb0(%arg0: i32, %arg1: i32):
    %0 = "scf.if"(%5#2) ({
      %1 = "arith.addi"(%arg0, %arg1) : (i32, i32) -> i32
      "scf.yield"(%4#2) : (i32) -> ()
    }, {
      "scf.yield"(%arg1) : (i32) -> ()
    }) : (i1) -> i32
    "func.return"(%0) : (i32) -> ()
  }) : () -> ()
}) {calyx.entrypoint = "main"} : () -> ()

Todo

I was not able to get the return assignments working as well as the Calyx control. After I added return success() in this line for scf::ifOp I thought everything will be built smoothly because I handled the condition argument, the then block, as well as the else block. I have also built the return assignment register, and since we already have buildOp(retOp here, I thought I don't need to worry about it.
However, the debug info is emitting the following error:

/circt/include/circt/Dialect/Calyx/CalyxLoweringUtils.h:431: TGroupOp circt::calyx::ComponentLoweringStateInterface::getEvaluatingGroup(mlir::Value) [with TGroupOp = circt::calyx::GroupInterface]: Assertion `it != valueGroupAssigns.end() && "No group evaluating value!"' failed.
PLEASE submit a bug report to https://github.com/llvm/circt and include the crash backtrace.
Stack dump:
0.	Program arguments: ./build/bin/circt-opt --lower-scf-to-calyx --mlir-print-ir-after-all test.mlir
Stack dump without symbol names (ensure you have llvm-symbolizer in your PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
0  circt-opt 0x0000564fe0fefc20 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) + 240
...
deleted the long bug report here
...
Aborted (core dumped)

Seems like getEvaluatingGroup is raising some errors. I have read the doc here but I'm still not entirely sure what it does. Which phase is it used in? Build-component/group-phase, or build-control-phase?

[cgyurgyik]

getEvaluatingGroup will return the group that assigns to the passed in value v.

For example [1],

    // 1. Retrieve the conditional SSA value.
    auto cond = whileOp.getConditionValue();
    // Get the group under which it is assigned.
    auto condGroup = getState<ComponentLoweringState>()
                         .getEvaluatingGroup<calyx::CombGroupOp>(cond);
    // Then, get its symbol (this is what a calyx::WhileOp builder needs).
    auto symbolAttr = FlatSymbolRefAttr::get(
        StringAttr::get(getContext(), condGroup.getSymName()));
    // Lastly, pass that into the calyx::WhileOp.
    return rewriter.create<calyx::WhileOp>(loc, cond, symbolAttr);
  }

You're trying to get the evaluating group of some SSA value that does not have a group, or wasn't properly register here. To make progress, I would suggest finding where this call is coming from, and what the value is.

1:

circt/lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

Lines 1476 to 1483 in d4bc36f

	/// Insert the while op itself.
	auto cond = whileOp.getConditionValue();
	auto condGroup = getState<ComponentLoweringState>()
	.getEvaluatingGroup<calyx::CombGroupOp>(cond);
	auto symbolAttr = FlatSymbolRefAttr::get(
	StringAttr::get(getContext(), condGroup.getSymName()));
	return rewriter.create<calyx::WhileOp>(loc, cond, symbolAttr);
	}

[jiahanxie353]

Thanks for the advice! I got almost everything working now excepting Calyx control.

Right now I only have enabled ret_assign:

// -----// IR Dump After SCFToCalyx Failed (lower-scf-to-calyx) //----- //
"builtin.module"() ({
  "calyx.component"() ({
  ^bb0(%arg0: i32, %arg1: i32, %arg2: i1, %arg3: i1, %arg4: i1, %arg5: i32, %arg6: i1):
    %0 = "hw.constant"() {value = true} : () -> i1
    %1:6 = "calyx.register"() {sym_name = "cond_arg_reg"} : () -> (i1, i1, i1, i1, i1, i1)
    %2:6 = "calyx.register"() {sym_name = "else_yield_0_reg"} : () -> (i32, i1, i1, i1, i32, i1)
    %3:6 = "calyx.register"() {sym_name = "then_yield_0_reg"} : () -> (i32, i1, i1, i1, i32, i1)
    %4:3 = "calyx.std_add"() {sym_name = "std_add_0"} : () -> (i32, i32, i32)
    %5:3 = "calyx.std_slt"() {sym_name = "std_slt_0"} : () -> (i32, i32, i1)
    %6:6 = "calyx.register"() {sym_name = "ret_arg0_reg"} : () -> (i32, i1, i1, i1, i32, i1)
    "calyx.wires"() ({
      "calyx.assign"(%1#0, %5#2) : (i1, i1) -> ()
      "calyx.assign"(%arg5, %6#4) : (i32, i32) -> ()
      "calyx.group"() ({
        "calyx.assign"(%3#0, %4#0) : (i32, i32) -> ()
        "calyx.assign"(%3#1, %0) : (i1, i1) -> ()
        "calyx.group_done"(%3#5) : (i1) -> ()
      }) {sym_name = "assign_then_group"} : () -> ()
      "calyx.group"() ({
        "calyx.assign"(%2#0, %arg1) : (i32, i32) -> ()
        "calyx.assign"(%2#1, %0) : (i1, i1) -> ()
        "calyx.group_done"(%2#5) : (i1) -> ()
      }) {sym_name = "assign_else_group"} : () -> ()
      "calyx.group"() ({
        "calyx.assign"(%6#0, %3#4) : (i32, i32) -> ()
        "calyx.assign"(%6#1, %0) : (i1, i1) -> ()
        "calyx.group_done"(%6#5) : (i1) -> ()
      }) {sym_name = "ret_assign_0"} : () -> ()
    }) : () -> ()
    "calyx.control"() ({
      "calyx.seq"() ({
        "calyx.enable"() {groupName = @ret_assign_0} : () -> ()
      }) : () -> ()
    }) : () -> ()
  }) {function_type = (i32, i32, i1, i1, i1, i32, i1) -> (), portAttributes = [{}, {}, {clk}, {reset}, {go}, {}, {done}], portDirections = -32 : i7, portNames = ["in0", "in1", "clk", "reset", "go", "out0", "done"], sym_name = "main", toplevel} : () -> ()
}) {calyx.entrypoint = "main"} : () -> ()

And the verifier is complaining

error: 'calyx.group' op with name: "assign_then_group" is unused in the control execution schedule
  %1 = scf.if %0 -> i32 {
       ^
test.mlir:3:8: note: see current operation: 
"calyx.group"() ({
  "calyx.assign"(%3#0, %4#0) : (i32, i32) -> ()
  "calyx.assign"(%3#1, %0) : (i1, i1) -> ()
  "calyx.group_done"(%3#5) : (i1) -> ()
}) {sym_name = "assign_then_group"} : () -> ()

To proceed, I guess I have to support scf.ifOp in LoopScheduleToCalyx to schedule the correct if-else control?

[rachitnigam]

No, it’s complaining that compilation generated a group that is not used by the calyx control program. This means you generated the group but didn’t add it to the control program

[jiahanxie353]

This means you generated the group but didn’t add it to the control program

I see. Can you point me to the right direction of adding to the control program? I thought I should use addBlockScheduleable but it's not working.

[cgyurgyik]

This means you generated the group but didn’t add it to the control program

I see. Can you point me to the right direction of adding to the control program? I thought I should use addBlockScheduleable but it's not working.

I am not at the computer but IIRC there is some BuildCfgControl graph no?

[jiahanxie353]

I am not at the computer but IIRC there is some BuildCfgControl graph no?

Yes! Working on it!

[jiahanxie353]

Hi, I've working on adding control to scf::IfOp and faced some issues during runOnOperation.

When I'm recurseInlineCombGroups here, which then getEvaluateGroup here, it could not find the evaluating group.

Specifically, it could not find the evaluating group for: %4:3 = "calyx.std_add"() {sym_name = "std_add_0"} : () -> (i32, i32, i32), which is the %2 = arith.addi %arg0, %arg1 : i32 in the .mlir file. Moreover, the originGroup here is:

"calyx.group"() ({
  "calyx.assign"(%3#0, %4#0) : (i32, i32) -> ()
  "calyx.assign"(%3#1, %0) : (i1, i1) -> ()
  "calyx.group_done"(%3#5) : (i1) -> ()
}) {sym_name = "assign_then_group"} : () -> ()
$1 = void

if you this information is helpful.

However, as I have print statements whenever I register the evaluating group, I'm pretty sure that I registered this calyx.std_add since the output prints out:

Registering evaluating group: 
"calyx.comb_group"() ({
  "calyx.assign"(%0#0, %arg0) : (i32, i32) -> ()
  "calyx.assign"(%0#1, %arg1) : (i32, i32) -> ()
}) {sym_name = "bb0_1"} : () -> ()
For the value: 
%0:3 = "calyx.std_add"() {sym_name = "std_add_0"} : () -> (i32, i32, i32)

I've been stuck for a while an any insight will be much appreciated.
As a side question, what are #0, #1, ..., #5 for a calyx.std_add respectively?

Thank you!

[jiahanxie353]

Hi folks,

Just finished almost everything of supporting scf.ifOp and cleaned up the commit history as well.

One thing that is still missing is lateSSAReplacement here.
Since I made both thenYieldRegs and elseYieldRegs, I'm not sure how to get the correct registers based on condition.

Any suggestions? Should I only be using yieldRegs instead of splitting them to thenRegs and elseRegs?

[mikeurbach]

Any suggestions? Should I only be using yieldRegs instead of splitting them to thenRegs and elseRegs?

I think it would be possible to update lateSSAReplacement to look at the data you added to both thenRegs and elseRegs. I need to take another look at the PR to see what makes sense, but in general, it should be fine to adapt lateSSAReplacement to the changes you've made here.

[darthscsi]

If the find is only used in an assert, it's fine to do

assert(regs.count(idx) ....);
return regs[idx];

[darthscsi]

nit: Again, we aren't worried about the assertion path and generally lean towards shorter code.

[jiahanxie353]

Marking ready for review again. And pretty sure it's correct this time.

[rachitnigam]

LGTM on my end! @cgyurgyik any final C++ comments?

[cgyurgyik]

LGTM, nice work. Just one small nit.

[cgyurgyik]

Nit: This is confusing since you already have a res variable above. Can you rename this please?