[mlir][emitc][cf] add 'cf.switch' support in CppEmitter

[EtoAndruwa]

Added 'switch' operation from cf dialect to the CppEmitter.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-emitc

Author: Andrey Timonin (EtoAndruwa)

Changes

Added 'switch' operation from cf dialect to the CppEmitter.

---

Full diff: https://github.com/llvm/llvm-project/pull/101478.diff

2 Files Affected:

• (modified) mlir/lib/Target/Cpp/TranslateToCpp.cpp (+37-2)
• (added) mlir/test/Target/Cpp/switch.mlir (+37)

diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index 1dadb9dd691e7..a05cbccf1f429 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -774,6 +774,41 @@ static LogicalResult printOperation(CppEmitter &emitter,
   return printBinaryOperation(emitter, operation, "||");
 }
 
+static LogicalResult printOperation(CppEmitter &emitter,
+                                    cf::SwitchOp switchOp) {
+  raw_indented_ostream &os = emitter.ostream();
+  auto iteratorCaseValues = (*switchOp.getCaseValues()).begin();
+  auto iteratorCaseValuesEnd = (*switchOp.getCaseValues()).end();
+
+  os << "\nswitch(" << emitter.getOrCreateName(switchOp.getFlag()) << ") {";
+
+  for (const auto caseBlock : switchOp.getCaseDestinations()) {
+    if (iteratorCaseValues == iteratorCaseValuesEnd)
+      return switchOp.emitOpError("case's value is absent for case block");
+
+    os << "\ncase " << *(iteratorCaseValues++) << ": {\n";
+    os.indent() << "goto ";
+
+    if (!(emitter.hasBlockLabel(*caseBlock)))
+      return switchOp.emitOpError("unable to find label for case block");
+    os << emitter.getOrCreateName(*caseBlock) << ";\n";
+
+    os.unindent() << "}";
+  }
+
+  os << "\ndefault: {\n";
+  os.indent() << "goto ";
+
+  if (!(emitter.hasBlockLabel(*switchOp.getDefaultDestination())))
+    return switchOp.emitOpError("unable to find label for default block");
+  os << emitter.getOrCreateName(*switchOp.getDefaultDestination()) << ";\n";
+
+  os.unindent() << "}\n";
+  os << "}\n";
+
+  return success();
+}
+
 static LogicalResult printOperation(CppEmitter &emitter, emitc::ForOp forOp) {
 
   raw_indented_ostream &os = emitter.ostream();
@@ -998,7 +1033,7 @@ static LogicalResult printFunctionBody(CppEmitter &emitter,
       // trailing semicolon is handled within the printOperation function.
       bool trailingSemicolon =
           !isa<cf::CondBranchOp, emitc::DeclareFuncOp, emitc::ForOp,
-               emitc::IfOp, emitc::VerbatimOp>(op);
+               cf::SwitchOp, emitc::IfOp, emitc::VerbatimOp>(op);
 
       if (failed(emitter.emitOperation(
               op, /*trailingSemicolon=*/trailingSemicolon)))
@@ -1496,7 +1531,7 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
           // Builtin ops.
           .Case<ModuleOp>([&](auto op) { return printOperation(*this, op); })
           // CF ops.
-          .Case<cf::BranchOp, cf::CondBranchOp>(
+          .Case<cf::BranchOp, cf::CondBranchOp, cf::SwitchOp>(
               [&](auto op) { return printOperation(*this, op); })
           // EmitC ops.
           .Case<emitc::AddOp, emitc::ApplyOp, emitc::AssignOp,
diff --git a/mlir/test/Target/Cpp/switch.mlir b/mlir/test/Target/Cpp/switch.mlir
new file mode 100644
index 0000000000000..68ba2c90dde6d
--- /dev/null
+++ b/mlir/test/Target/Cpp/switch.mlir
@@ -0,0 +1,37 @@
+// RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s
+
+func.func @switch_func(%a: i32, %b: i32, %c: i32) -> () {
+    cf.switch %b : i32, [
+    default: ^bb1(%a : i32),
+    42: ^bb1(%b : i32),
+    43: ^bb2(%c : i32),
+    44: ^bb3(%c : i32)
+    ]
+
+    ^bb1(%x1 : i32) :
+        %y1 = "emitc.add" (%x1, %x1) : (i32, i32) -> i32
+        return 
+
+    ^bb2(%x2 : i32) :
+        %y2 = "emitc.sub" (%x2, %x2) : (i32, i32) -> i32
+        return 
+
+    ^bb3(%x3 : i32) :
+        %y3 = "emitc.mul" (%x3, %x3) : (i32, i32) -> i32
+        return 
+}
+// CHECK: void switch_func(int32_t [[V0:[^ ]*]], int32_t [[V1:[^ ]*]], int32_t [[V2:[^ ]*]]) {
+// CHECK: switch([[V1:[^ ]*]]) {
+// CHECK-NEXT: case 42: {
+// CHECK-NEXT: goto label2;
+// CHECK-NEXT: }
+// CHECK-NEXT: case 43: {
+// CHECK-NEXT: goto label3;
+// CHECK-NEXT: }
+// CHECK-NEXT: case 44: {
+// CHECK-NEXT: goto label4;
+// CHECK-NEXT: }
+// CHECK-NEXT: default: {
+// CHECK-NEXT: goto label2;
+// CHECK-NEXT: }
+// CHECK-NEXT: }

[simon-camp]

Overall this looks good to me.

You should also add a case in this check, otherwise you get a trailing semicolon after the switch body (I think, I haven't run the code).

[simon-camp]

Move this function up, directly below the cf.cond_br function.

[EtoAndruwa]

Fixed.

[simon-camp]

You need to copy the operands into the variables for the successor block arguments like here.

[EtoAndruwa]

Thank you! That was a critical error. Fixed.

[EtoAndruwa]

I will fix the test right now too. Missed it.

[simon-camp]

Order this by dialect name, please.

[EtoAndruwa]

Fixed.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[EtoAndruwa]

I think the CI check for code formatting failed without any reason. I just checked code formatting locally.

[simon-camp]

I'll wait on the opinion of @aniragil and/or @mgehre-amd before approving.

On the one hand we've removed support for several non EmitC ops from the emitter in recent months by converting these to the Emitc dialect. On the other hand we don't have EmitC specific ops for branch like instructions the cf dialect could be lowered to yet.

[marbre]

I'll wait on the opinion of @aniragil and/or @mgehre-amd before approving.

On the one hand we've removed support for several non EmitC ops from the emitter in recent months by converting these to the Emitc dialect. On the other hand we don't have EmitC specific ops for branch like instructions the cf dialect could be lowered to yet.

I agree with @simon-camp here, not sure if we want to add support for further non-EmitC ops to the emitter. I would therefore love to hear @mgehre-amd's and @aniragil's option. If we want to add this, please also also adjust https://github.com/llvm/llvm-project/blob/main/mlir/docs/Dialects/emitc.md accordingly as part of your PR.

[aniragil]

I'll wait on the opinion of @aniragil and/or @mgehre-amd before approving.
On the one hand we've removed support for several non EmitC ops from the emitter in recent months by converting these to the Emitc dialect. On the other hand we don't have EmitC specific ops for branch like instructions the cf dialect could be lowered to yet.

I agree with @simon-camp here, not sure if we want to add support for further non-EmitC ops to the emitter. I would therefore love to hear @mgehre-amd's and @aniragil's option. If we want to add this, please also also adjust https://github.com/llvm/llvm-project/blob/main/mlir/docs/Dialects/emitc.md accordingly as part of your PR.

I share @simon-camp's and @marbre's concerns about extending the translator.
How about the following alternative then: Lower cf.switch to a new emitc.switch op (which would also be useful for lowering scf.switch). Within the emitc.switch cases we'll generate cf.br ops which the translator already supports. The cf.br ops will carry the cf.switch cases' block arguments. Similarly, we can lower cf.cond_br to the existing emitc.if op, leaving the translator to handle only cf.br until we model blocks/labels and goto's propely.

We can implement this in either order:

• First move the existing cf.cond_br support to a new CFToEmitC lowering pass, then add an emitc.switch op and extend the pass to lower cf.switch into it.
• First add an emitc.switch op with a new CFToEmitC lowering pass which uses it for cf.switch, then move the existing cf.cond_br support there.

WDYT?

[marbre]

I share @simon-camp's and @marbre's concerns about extending the translator. How about the following alternative then: Lower cf.switch to a new emitc.switch op (which would also be useful for lowering scf.switch). Within the emitc.switch cases we'll generate cf.br ops which the translator already supports. The cf.br ops will carry the cf.switch cases' block arguments. Similarly, we can lower cf.cond_br to the existing emitc.if op, leaving the translator to handle only cf.br until we model blocks/labels and goto's propely.

We can implement this in either order:

• First move the existing cf.cond_br support to a new CFToEmitC lowering pass, then add an emitc.switch op and extend the pass to lower cf.switch into it.
• First add an emitc.switch op with a new CFToEmitC lowering pass which uses it for cf.switch, then move the existing cf.cond_br support there.

WDYT?

If an emitc.switch option is useful and doesn't not just replicated cf.switch but can instead be used to lower to from scf.index_switch (I assume you @aniragil were referring to that), I am highly in favor of a new EmitC op. Great suggestion @aniragil!
However, I don't think that we necessarily need to generate cf.br ops within an emitc.switch op. For me it's fine to partly duplicate logic in the translator, while keeping code duplications as low as possible, and add complete support for the emitc.switch op to the translator. With that, it could go more into a direction where we could entirely remove support for the cf dialect one day.

[simon-camp]

I'll wait on the opinion of @aniragil and/or @mgehre-amd before approving.
On the one hand we've removed support for several non EmitC ops from the emitter in recent months by converting these to the Emitc dialect. On the other hand we don't have EmitC specific ops for branch like instructions the cf dialect could be lowered to yet.

I agree with @simon-camp here, not sure if we want to add support for further non-EmitC ops to the emitter. I would therefore love to hear @mgehre-amd's and @aniragil's option. If we want to add this, please also also adjust https://github.com/llvm/llvm-project/blob/main/mlir/docs/Dialects/emitc.md accordingly as part of your PR.

I share @simon-camp's and @marbre's concerns about extending the translator. How about the following alternative then: Lower cf.switch to a new emitc.switch op (which would also be useful for lowering scf.switch). Within the emitc.switch cases we'll generate cf.br ops which the translator already supports. The cf.br ops will carry the cf.switch cases' block arguments. Similarly, we can lower cf.cond_br to the existing emitc.if op, leaving the translator to handle only cf.br until we model blocks/labels and goto's propely.

We can implement this in either order:

• First move the existing cf.cond_br support to a new CFToEmitC lowering pass, then add an emitc.switch op and extend the pass to lower cf.switch into it.
• First add an emitc.switch op with a new CFToEmitC lowering pass which uses it for cf.switch, then move the existing cf.cond_br support there.

WDYT?

I don't think this that's going to work as is. You can't branch from a region to a basic block in a parent region (I think). Additionally cf.cond_br and cf.switch are terminators whereas emitc.if is not. So we would lose the terminator by simply lowering these ops to ifs.

The end goal is fine, I just can't estimate how much work this involves.

[marbre]

I'll wait on the opinion of @aniragil and/or @mgehre-amd before approving.
On the one hand we've removed support for several non EmitC ops from the emitter in recent months by converting these to the Emitc dialect. On the other hand we don't have EmitC specific ops for branch like instructions the cf dialect could be lowered to yet.

I agree with @simon-camp here, not sure if we want to add support for further non-EmitC ops to the emitter. I would therefore love to hear @mgehre-amd's and @aniragil's option. If we want to add this, please also also adjust https://github.com/llvm/llvm-project/blob/main/mlir/docs/Dialects/emitc.md accordingly as part of your PR.

I share @simon-camp's and @marbre's concerns about extending the translator. How about the following alternative then: Lower cf.switch to a new emitc.switch op (which would also be useful for lowering scf.switch). Within the emitc.switch cases we'll generate cf.br ops which the translator already supports. The cf.br ops will carry the cf.switch cases' block arguments. Similarly, we can lower cf.cond_br to the existing emitc.if op, leaving the translator to handle only cf.br until we model blocks/labels and goto's propely.
We can implement this in either order:

• First move the existing cf.cond_br support to a new CFToEmitC lowering pass, then add an emitc.switch op and extend the pass to lower cf.switch into it.
• First add an emitc.switch op with a new CFToEmitC lowering pass which uses it for cf.switch, then move the existing cf.cond_br support there.

WDYT?

I don't think this that's going to work as is. You can't branch from a region to a basic block in a parent region (I think). Additionally cf.cond_br and cf.switch are terminators whereas emitc.if is not. So we would lose the terminator by simply lowering these ops to ifs.

The end goal is fine, I just can't estimate how much work this involves.

But you're in the team that thinks that an emitc.switch op would be of value but with dedicated support in the emitter?

[simon-camp]

We can add a switch op, but it shouldn't be a terminator in my opinion. So we would be able to lower scf.index_switch to it, but we couldn't use it for cf.switch (as of now).

[EtoAndruwa]

Hello everyone! I am grateful for this productive discussion. It was great to hear of all of you. Should I close the PR and start the issue for emitc.switch op?

[aniragil]

I don't think this that's going to work as is. You can't branch from a region to a basic block in a parent region (I think). Additionally cf.cond_br and cf.switch are terminators whereas emitc.if is not. So we would lose the terminator by simply lowering these ops to ifs.

Hmm, good points. So our options is either to

(a) First replace translator support for cf by lowering cf into emitc, e.g. by adding a pair of emitc.label, emitc.goto ops, then add an emitc.switch op and support cf.switch.

(b) First support cf.switch in the translator, then add an emitc.switch op by lowering scf.index_switch into it, then move all cf support from the translator to a lowering pass.

As @marbre mentioned, option (a) is better as it only involves "going forward" with emitc's design, tought it would take a bit more time. Also, option (b) risks changing the emitted code later when we move cf lowering to a pass (e.g. if we end up lowering cf.switch to cascading block & cond_br ops for whatever reason). So if we can come up in a reasonable time with a design for lowering branches and blocks to emitc I'd go for this option.
That being said, option (b) doesn't seem like a big step back as it's a clean extension of the existing mechanism and I'm OK with changing the emitted code later (though I doubt it would be needed), so I'm OK with going with option (b) if @simon-camp and @marbre expect lowering branches and blocks to take long or be complex.

[kchibisov]

I'd add some background, since @EtoAndruwa works on my team.

The only reason we went for cf.switch is that emitc already has cf.br and cf.cond_br. In general, I'd favor an emitc.switch operation which always lowers without goto and uses simple break, so the case labels are not terminators and you can not jump from them, so it'll look more like emitc.if, from what I understand in terms of child basic blocks.

In general, I'd prefer if cf dialect will lower to emitc dialect first and then be used by emitter, since it'll result in improved code gen and you won't need to account for cf when writing passes, since you can require lowering to emitc first.

We can add a switch op, but it shouldn't be a terminator in my opinion. So we would be able to lower scf.index_switch to it, but we couldn't use it for cf.switch (as of now).

I wonder how the scf.yield will be lowered (though, I guess you can just specify that you can not yield result and say that each case should end with emitc.break, which yields nothing), though, you can likely define a variable just before the switch and assign to it, but all in all, scf.index_swicth like emitc.switch looks more appealing to us for already said goto stuff above, so +1 here.

[simon-camp]

I'd add some background, since @EtoAndruwa works on my team.

The only reason we went for cf.switch is that emitc already has cf.br and cf.cond_br. In general, I'd favor an emitc.switch operation which always lowers without goto and uses simple break, so the case labels are not terminators and you can not jump from them, so it'll look more like emitc.if, from what I understand in terms of child basic blocks.

In general, I'd prefer if cf dialect will lower to emitc dialect first and then be used by emitter, since it'll result in improved code gen and you won't need to account for cf when writing passes, since you can require lowering to emitc first.

We can add a switch op, but it shouldn't be a terminator in my opinion. So we would be able to lower scf.index_switch to it, but we couldn't use it for cf.switch (as of now).

I wonder how the scf.yield will be lowered (though, I guess you can just specify that you can not yield result and say that each case should end with emitc.break, which yields nothing), though, you can likely define a variable just before the switch and assign to it, but all in all, scf.index_swicth like emitc.switch looks more appealing to us for already said goto stuff above, so +1 here.

That's exactly how scf.if is lowered to emitc.if; by declaring result variables up front and lowering scf.yield into assignments and an empty emitc.yield as terminator.

Do your cf.switch ops come from a lowering of scf.index_switch ops? Then adding an emitc.switch op and adding a pattern to SCFToEmitC conversion would be easiest, I guess.

[kchibisov]

Do your cf.switch ops come from a lowering of scf.index_switch ops? Then adding an emitc.switch op and adding a pattern to SCFToEmitC conversion would be easiest, I guess.

Ah, no, we start from emitc + our dialect directly, most of the time. We just need a way to declare something that will result in switch pattern, whether it'll be through the cf.switch, scf.index_switch or emitc.switch doesn't really matter, though, the version with just break; patterns will be preferred.

I just know that we'll need to patch emitter in the end of the day, so it should go upstream.

[aniragil]

I'd add some background, since @EtoAndruwa works on my team.

The only reason we went for cf.switch is that emitc already has cf.br and cf.cond_br. In general, I'd favor an emitc.switch operation which always lowers without goto and uses simple break, so the case labels are not terminators and you can not jump from them, so it'll look more like emitc.if, from what I understand in terms of child basic blocks.

Thanks for the context, this indeed clarifies things!
Agreed, better to keep control flow structured where possible. The emitc.switch I had in mind would follow the semantics of scf.index_switch, which is less than what a C switch statement can do, e.g. fall-through, but captures exactly what MLIR's scf dialect models (we followed the same logic when we added emitc.if and emitc.for). So it makes sense to me to define emitc.switch as terminating each case with a break statement.

In general, I'd prefer if cf dialect will lower to emitc dialect first and then be used by emitter, since it'll result in improved code gen and you won't need to account for cf when writing passes, since you can require lowering to emitc first.

Agreed. If some emitc pass must take unstructured control flow into account it could either analyze the emitc control flow or be executed before cf-to-emitc.

I wonder how the scf.yield will be lowered (though, I guess you can just specify that you can not yield result and say that each case should end with emitc.break, which yields nothing), though, you can likely define a variable just before the switch and assign to it, but all in all, scf.index_swicth like emitc.switch looks more appealing to us for already said goto stuff above, so +1 here.

We lower scf.yield's return/iter args as part of scf.if/scf.for using emitc.variables which get defined before the respective emitc.if/emitc.for op and get assigned to in the then/else/body regions (emitc.if and emitc.for indeed cannot return any value). A similar lowering should work for emitc.switch, where each case would end with a nullary emitc.yield.

[aniragil]

Should I close the PR and start the issue for emitc.switch op?

I'd separate between these issues:

• There seems to be a consensus that an emitc.switch op with lowering from scf.index_switch would be a good addition to EmitC in and of itself, so would be great if you'd upload such a patch.

• This PR started a good discussion regarding what to do with the cf dialect and provides an implementation for one of the alternatives, so would be good to keep around for further discussion.