[mlir][emitc] Don't emit extra semicolon after bracket

[kchibisov]

Extra semicolons were emitted for operations that should never have them, since not every place was checking whether semicolon would be actually needed.

Thus change the emitOperation to ignore trailingSemicolon field for such operations.

[llvmbot]

@llvm/pr-subscribers-mlir-emitc

@llvm/pr-subscribers-mlir

Author: Kirill Chibisov (kchibisov)

Changes

Extra semicolons were emitted for operations that should never have them, since not every place was checking whether semicolon would be actually needed.

Thus change the emitOperation to ignore trailingSemicolon field for such operations.

---

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

2 Files Affected:

• (modified) mlir/lib/Target/Cpp/TranslateToCpp.cpp (+10-10)
• (added) mlir/test/Target/Cpp/no_extra_semicolon.mlir (+18)

diff --git a/mlir/lib/Target/Cpp/TranslateToCpp.cpp b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
index d26adec500a113..dba9a625382def 100644
--- a/mlir/lib/Target/Cpp/TranslateToCpp.cpp
+++ b/mlir/lib/Target/Cpp/TranslateToCpp.cpp
@@ -120,6 +120,10 @@ struct CppEmitter {
   LogicalResult emitAttribute(Location loc, Attribute attr);
 
   /// Emits operation 'op' with/without training semicolon or returns failure.
+  ///
+  /// If the operation should not be followed by semicolon, like VerbatimOp,
+  /// the `trailingSemicolon` argument is ignore and semicolon is not
+  /// emitted.
   LogicalResult emitOperation(Operation &op, bool trailingSemicolon);
 
   /// Emits type 'type' or returns failure.
@@ -1036,16 +1040,7 @@ static LogicalResult printFunctionBody(CppEmitter &emitter,
         return failure();
     }
     for (Operation &op : block.getOperations()) {
-      // When generating code for an emitc.if or cf.cond_br op no semicolon
-      // needs to be printed after the closing brace.
-      // When generating code for an emitc.for and emitc.verbatim op, printing a
-      // trailing semicolon is handled within the printOperation function.
-      bool trailingSemicolon =
-          !isa<cf::CondBranchOp, emitc::DeclareFuncOp, emitc::ForOp,
-               emitc::IfOp, emitc::SwitchOp, emitc::VerbatimOp>(op);
-
-      if (failed(emitter.emitOperation(
-              op, /*trailingSemicolon=*/trailingSemicolon)))
+      if (failed(emitter.emitOperation(op, /*trailingSemicolon=*/true)))
         return failure();
     }
   }
@@ -1607,6 +1602,11 @@ LogicalResult CppEmitter::emitOperation(Operation &op, bool trailingSemicolon) {
        shouldBeInlined(cast<emitc::ExpressionOp>(op))))
     return success();
 
+  // Never emit semicolon for operations that end with } or opaque.
+  trailingSemicolon &=
+      !isa<cf::CondBranchOp, emitc::DeclareFuncOp, emitc::ForOp, emitc::IfOp,
+           emitc::SwitchOp, emitc::VerbatimOp, emitc::IncludeOp>(op);
+
   os << (trailingSemicolon ? ";\n" : "\n");
 
   return success();
diff --git a/mlir/test/Target/Cpp/no_extra_semicolon.mlir b/mlir/test/Target/Cpp/no_extra_semicolon.mlir
new file mode 100644
index 00000000000000..e6e0c58daf4e42
--- /dev/null
+++ b/mlir/test/Target/Cpp/no_extra_semicolon.mlir
@@ -0,0 +1,18 @@
+// RUN: mlir-translate -mlir-to-cpp %s | FileCheck %s
+// RUN: mlir-translate -mlir-to-cpp -declare-variables-at-top %s | FileCheck %s
+
+func.func @no_extra_semicolon(%arg0: i1) {
+  emitc.if %arg0 {
+    emitc.if %arg0 {
+    }
+    emitc.verbatim "return;"
+  }
+  return
+}
+// CHECK: void test_if(bool [[V0:[^ ]*]]) {
+// CHECK-NEXT: if ([[V0]]) {
+// CHECK-NEXT: if ([[V0]]) {
+// CHECK-NEXT: }
+// CHECK-NEXT: return;
+// CHECK-NEXT: }
+// CHECK-NEXT: return;

[mgehre-amd]

Thanks, this moves the code in the right direction!

I think we should eventually get rid of the trailingSemicolon parameter of the emitOperation function; I still find it confusion.
What might help if we eventually split this function into emitStatement and emitExpression.
Statements are terminated with a semicolon (except for special op types like VerbatimOp), where expressions don't have that. We can then replace all calls to emitOperation(op, /trailingSemicolon/=true)byemitStatement(op)and all calls toemitExpression(op, /trailingSemicolon/=true). emitStatementwould contain the dispatch for all statement-ops likeIfOp, VerbatimOpetc, and otherwise callemitExpression()followed by emitting asemicolon. emitExpressionwould contain the dispatch for all expression ops likeAddOp, TernaryOp` etc.
But this is for another PR.

[mgehre-amd]

Looks good!

[aniragil]

Looks good, just some minor nits.

[aniragil]

Another nit (keep ops in lexicographic order)

[marbre]

Thanks for the patch and the other reviewers for the first rounds! Looks good to me, just a minor NIT.

[marbre]

Thanks! In case you don't have write access, let me know and I can merged on your behalf.

[kchibisov]

I don't have write access.

[marbre]

I don't have write access.

At the moment this would be squashed with just your email. Do you want to add your name before I merge?

[kchibisov]

@marbre I'm not sure what you're talking about, the commit has all the correct formatting + name/etc? There's also nothing to squash, since it's a single commit.

Do you want me to do something extra, since nothing changed between how I was submitting things in the past and now.

[kchibisov]

You can see it like that https://patch-diff.githubusercontent.com/raw/llvm/llvm-project/pull/122464.patch , you can see that the name is clearly there.

[marbre]

You can see it like that https://patch-diff.githubusercontent.com/raw/llvm/llvm-project/pull/122464.patch , you can see that the name is clearly there.

I might have got confused by the GitHub UI which only shows the email address for the squash commit. Merged it, thanks again for the fix!
With regards to squashing, even single commit PRs get squashed and thus, among other things, link back to the PR and the related discussion.