[SCFToCalyx] If op with sequential condition #7687

jiahanxie353 · 2024-10-09T21:17:51Z

This patch addresses #7682. It supports IfOp whose condition check involves sequential operations by enabling buildLibraryOp to create GroupOp.

jiahanxie353 · 2024-10-09T21:33:13Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

@@ -240,13 +255,36 @@ class ForLoopLoweringStateInterface
  }
 };

+class PipeOpLoweringStateInterface {


I'm not very satisfied with this approach though, having to introduce another interface to "remember" the register set for the sequential/pipelined library operator.

Well, we'll definitely need some way to recall this mapping, since it isn't explicitly stated in the IR. If this is only necessary for lowering conditionals in if statements, then I think it would make more sense in the IfLoweringStateInterface.

With that said, I don't think this is the only Calyx control flow that will require this special treatment, i.e., while should be similar. What other approaches are you thinking about?

In any case, it would be nice to add documentation on why this interface exists.

cgyurgyik

Just to check my understanding, this PR is ensuring that if the condition of a SCF if op depends on some sequential operation, we create a separate group and save its value in a register to be used in the Calyx if op. Is that correct?

cgyurgyik · 2024-10-09T22:05:53Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

+    Operation *operation = op.getOperation();
+    assert(condReg.count(operation) == 0 &&
+           "A condition register was already set for this scf::IfOp!\n");
+    condReg[operation] = regOp;


To avoid the double lookup here (once for count and once for the [] operator, we can do:

auto [it, succeeded] = condReg.insert(std::make_pair(operation, regOp)); assert(succeeded && "...");

cgyurgyik · 2024-10-09T22:07:41Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

@@ -172,6 +186,7 @@ class IfLoweringStateInterface {
  }

 private:
+  DenseMap<Operation *, calyx::RegisterOp> condReg;


I know this hasn't been the norm, but it would sure be nice to describe why this data structure exists for future readers :-)

cgyurgyik · 2024-10-09T22:08:13Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

+class PipeOpLoweringStateInterface {
+public:
+  void setPipeResReg(Operation *op, calyx::RegisterOp reg) {
+    assert(isa<calyx::MultPipeLibOp>(op) || isa<calyx::DivUPipeLibOp>(op) ||


prefer: isa<calyx::MultPipeLibOp, calyx::DivUPipelibOp, ...>(op)

cgyurgyik · 2024-10-09T22:08:22Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

+    assert(isa<calyx::MultPipeLibOp>(op) || isa<calyx::DivUPipeLibOp>(op) ||
+           isa<calyx::DivSPipeLibOp>(op) || isa<calyx::RemUPipeLibOp>(op) ||
+           isa<calyx::RemSPipeLibOp>(op));
+    assert(resultRegs.count(op) == 0 &&


Similar comment as above.

cgyurgyik · 2024-10-09T22:12:28Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

+      if (isSequential)
+        rewriter.create<calyx::AssignOp>(op.getLoc(), dstOp.value(),
+                                         srcReg.getOut());
+      else
+        rewriter.create<calyx::AssignOp>(op.getLoc(), dstOp.value(),
+                                         op->getOperand(dstOp.index()));


Suggested change

if (isSequential)

rewriter.create<calyx::AssignOp>(op.getLoc(), dstOp.value(),

srcReg.getOut());

else

rewriter.create<calyx::AssignOp>(op.getLoc(), dstOp.value(),

op->getOperand(dstOp.index()));

auto srcOp = isSequential ? srcReg.getOut() : op->getOperand(dstOp.index());

rewriter.create<calyx::AssignOp>(op.getLoc(), dstOp.value(), srcOp);

Maybe this doesn't work, but I would try to limit the scope of your conditional.

cgyurgyik · 2024-10-09T22:17:44Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

+  }
+
+private:
+  DenseMap<Operation *, calyx::RegisterOp> resultRegs;


A comment would be nice here!

cgyurgyik · 2024-10-09T22:18:54Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

+    resultRegs[op] = reg;
+  }
+  // Get the register for a specific pipe operation
+  calyx::RegisterOp getPipeResReg(Operation *op) {


Similarly for this function, it is used twice but only because you condition on an entire function call rather than the one element that is different: the function argument. I think we should avoid introducing this unless you think it improves readability.

cgyurgyik · 2024-10-09T22:22:03Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

@@ -240,13 +255,36 @@ class ForLoopLoweringStateInterface
  }
 };

+class PipeOpLoweringStateInterface {


Well, we'll definitely need some way to recall this mapping, since it isn't explicitly stated in the IR. If this is only necessary for lowering conditionals in if statements, then I think it would make more sense in the IfLoweringStateInterface.

With that said, I don't think this is the only Calyx control flow that will require this special treatment, i.e., while should be similar. What other approaches are you thinking about?

In any case, it would be nice to add documentation on why this interface exists.

cgyurgyik · 2024-10-09T22:22:40Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

@@ -339,7 +377,12 @@ class BuildOpGroups : public calyx::FuncOpPartialLoweringPattern {
  /// source operation TSrcOp.
  template <typename TGroupOp, typename TCalyxLibOp, typename TSrcOp>
  LogicalResult buildLibraryOp(PatternRewriter &rewriter, TSrcOp op,
-                               TypeRange srcTypes, TypeRange dstTypes) const {
+                               TypeRange srcTypes, TypeRange dstTypes,
+                               calyx::RegisterOp srcReg = nullptr,


Please add comments about these.

cgyurgyik · 2024-10-09T22:25:46Z

test/Conversion/SCFToCalyx/convert_controlflow.mlir

+    %rem = arith.remui %arg0, %two : i32
+    %cond = arith.cmpi eq, %arg0, %rem : i32
+
+    %res = scf.if %cond -> i32 {


Nit: could we minimize what you're trying to test even further, e.g., do we need the else case, mem alloc, ...? It seems the critical piece is an if statement that uses the output of a sequential operation.

jiahanxie353 · 2024-10-10T00:16:00Z

Just to check my understanding, this PR is ensuring that if the condition of a SCF if op depends on some sequential operation, we create a separate group and save its value in a register to be used in the Calyx if op. Is that correct?

That's exactly right! :-)

And to reply the following comment as a whole:

With that said, I don't think this is the only Calyx control flow that will require this special treatment, i.e., while should be similar. What other approaches are you thinking about?

In any case, it would be nice to add documentation on why this interface exists.

Let me shamelessly copy your wording to describe the feature I'm trying to support in general:
This (and future) PR is ensuring that if the condition of a SCF if/while op depends on some sequential operation, we create a separate group and save its value in a register to be used in the Calyx if/while op.

I don't think this is the only Calyx control flow that will require this special treatment

So you are right, both if and while need it.

What other approaches are you thinking about?

Currently, my implementation has the following lowered result:

      calyx.group @bb0_0 {
        calyx.assign %std_remu_pipe_0.left = %in0 : i32
        calyx.assign %std_remu_pipe_0.right = %c2_i32 : i32
        calyx.assign %remui_0_reg.in = %std_remu_pipe_0.out_remainder : i32
        calyx.assign %remui_0_reg.write_en = %std_remu_pipe_0.done : i1
        %0 = comb.xor %std_remu_pipe_0.done, %true : i1
        calyx.assign %std_remu_pipe_0.go = %0 ? %true : i1
        calyx.group_done %remui_0_reg.done : i1
      }
      calyx.group @bb0_1 {
        calyx.assign %std_eq_0.left = %remui_0_reg.out : i32
        calyx.assign %std_eq_0.right = %remui_0_reg.out : i32
        calyx.assign %cmpi_0_reg.in = %std_eq_0.out : i1
        calyx.assign %cmpi_0_reg.write_en = %true : i1
        calyx.group_done %cmpi_0_reg.done : i1
      }
    calyx.control {
        calyx.seq {
          calyx.enable @bb0_0
          calyx.enable @bb0_1
          calyx.if %cmpi_0_reg.out {
              some operation

I wonder, is it possible to do:

      calyx.group @bb0_0 {
        calyx.assign %std_remu_pipe_0.left = %in0 : i32
        calyx.assign %std_remu_pipe_0.right = %c2_i32 : i32
        calyx.assign %remui_0_reg.in = %std_remu_pipe_0.out_remainder : i32
        calyx.assign %remui_0_reg.write_en = %std_remu_pipe_0.done : i1
        %0 = comb.xor %std_remu_pipe_0.done, %true : i1
        calyx.assign %std_remu_pipe_0.go = %0 ? %true : i1
        calyx.group_done %remui_0_reg.done : i1
      }
      calyx.comb_group @bb0_1 {
        calyx.assign %std_eq_0.left = %remui_0_reg.out : i32
        calyx.assign %std_eq_0.right = %remui_0_reg.out : i32
      }

    calyx.control {
        calyx.seq {
          calyx.enable @bb0_0
          calyx.if %std_eq_0.out with @bb0_1 {
              some operation

So that we can save a cycle, but I'm not sure what needs to be check with respect to the stability of %std_eq_0.out with @bb0_1

@rachitnigam recommends the first approach though, as he mentioned in the issue:

compute_cond; // computes rem and store in reg
if reg.out { ... } else { ... }

cgyurgyik · 2024-10-10T01:05:46Z

I wonder, is it possible to do:

      calyx.group @bb0_0 {
        calyx.assign %std_remu_pipe_0.left = %in0 : i32
        calyx.assign %std_remu_pipe_0.right = %c2_i32 : i32
        calyx.assign %remui_0_reg.in = %std_remu_pipe_0.out_remainder : i32
        calyx.assign %remui_0_reg.write_en = %std_remu_pipe_0.done : i1
        %0 = comb.xor %std_remu_pipe_0.done, %true : i1
        calyx.assign %std_remu_pipe_0.go = %0 ? %true : i1
        calyx.group_done %remui_0_reg.done : i1
      }
      calyx.comb_group @bb0_1 {
        calyx.assign %std_eq_0.left = %remui_0_reg.out : i32
        calyx.assign %std_eq_0.right = %remui_0_reg.out : i32
      }

    calyx.control {
        calyx.seq {
          calyx.enable @bb0_0
          calyx.if %std_eq_0.out with @bb0_1 {
              some operation

So that we can save a cycle, but I'm not sure what needs to be check with respect to the stability of %std_eq_0.out with @bb0_1

My first thought is this additional step could be enabled as a peephole optimization starting from the calyx.if, i.e.,

calyx.enable @bb0_1
calyx.if %cmpi_0_reg.out {

-->

calyx.enable @bb0_1
calyx.if %std_eq_0.out with @bb0_1 {

where we get rid of an intermediate register.

jiahanxie353 · 2024-10-10T01:15:24Z

lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

-                                       op->getOperand(dstOp.index()));
+
+    for (auto dstOp : enumerate(opInputPorts)) {
+      if (isSequential)


Bug, should be:

Suggested change

if (isSequential)

if (isPipeLibOp(dstOp.value()))

rewriter.create<calyx::AssignOp>(op.getLoc(), dstOp.value(),

srcReg.getOut());

Then seems like we are checking isPipeLibOp twice, once in a caller function, another in a callee..

jiahanxie353 requested review from mikeurbach and mortbopet as code owners October 9, 2024 21:17

jiahanxie353 force-pushed the if-seq-cond branch from cb4f02b to b42c4a0 Compare October 9, 2024 21:28

support if op when its condition check is not combinational

4d83a5c

jiahanxie353 force-pushed the if-seq-cond branch from b42c4a0 to 4d83a5c Compare October 9, 2024 21:29

jiahanxie353 added the Calyx The Calyx dialect label Oct 9, 2024

jiahanxie353 requested review from rachitnigam and cgyurgyik October 9, 2024 21:29

jiahanxie353 commented Oct 9, 2024

View reviewed changes

small fix

7fc6c93

jiahanxie353 force-pushed the if-seq-cond branch from 700af35 to 7fc6c93 Compare October 9, 2024 22:08

cgyurgyik reviewed Oct 9, 2024

View reviewed changes

jiahanxie353 commented Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SCFToCalyx] If op with sequential condition #7687

[SCFToCalyx] If op with sequential condition #7687

jiahanxie353 commented Oct 9, 2024

jiahanxie353 Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik left a comment

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

cgyurgyik Oct 9, 2024

jiahanxie353 commented Oct 10, 2024

cgyurgyik commented Oct 10, 2024

jiahanxie353 Oct 10, 2024

[SCFToCalyx] If op with sequential condition #7687

Are you sure you want to change the base?

[SCFToCalyx] If op with sequential condition #7687

Conversation

jiahanxie353 commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgyurgyik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiahanxie353 commented Oct 10, 2024

cgyurgyik commented Oct 10, 2024

Choose a reason for hiding this comment