[CIR] Add transform test for cir-flatten-cfg #130861

andykaylor · 2025-03-11T23:23:56Z

A previous change added the cir-flatten-cfg transform and tested it by lowering a function with nested scopes to LLVM IR. This change adds support for invoking the cir-flatten-cfg pass from the cir-opt tool and adds a new test to verify that functionality in isolation.

llvmbot · 2025-03-11T23:24:27Z

@llvm/pr-subscribers-clangir

@llvm/pr-subscribers-clang

Author: Andy Kaylor (andykaylor)

Changes

A previous change added the cir-flatten-cfg transform and tested it by lowering a function with nested scopes to LLVM IR. This change adds support for invoking the cir-flatten-cfg pass from the cir-opt tool and adds a new test to verify that functionality in isolation.

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

3 Files Affected:

(added) clang/test/CIR/Transforms/scope.cir (+58)
(modified) clang/tools/cir-opt/CMakeLists.txt (+1)
(modified) clang/tools/cir-opt/cir-opt.cpp (+5)

diff --git a/clang/test/CIR/Transforms/scope.cir b/clang/test/CIR/Transforms/scope.cir
new file mode 100644
index 0000000000000..d4fc957884a43
--- /dev/null
+++ b/clang/test/CIR/Transforms/scope.cir
@@ -0,0 +1,58 @@
+// RUN: cir-opt %s -cir-flatten-cfg -o - | FileCheck %s
+
+module {
+  cir.func @foo() {
+    cir.scope {
+      %0 = cir.alloca !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>, ["a", init] {alignment = 4 : i64}
+      %1 = cir.const #cir.int<4> : !cir.int<u, 32>
+      cir.store %1, %0 : !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>
+    }
+    cir.return
+  }
+// CHECK:  cir.func @foo() {
+// CHECK:    cir.br ^bb1
+// CHECK:  ^bb1:  // pred: ^bb0
+// CHECK:    %0 = cir.alloca !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>, ["a", init] {alignment = 4 : i64}
+// CHECK:    %1 = cir.const #cir.int<4> : !cir.int<u, 32>
+// CHECK:    cir.store %1, %0 : !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>
+// CHECK:    cir.br ^bb2
+// CHECK:  ^bb2:  // pred: ^bb1
+// CHECK:    cir.return
+// CHECK:  }
+
+  // Should drop empty scopes.
+  cir.func @empty_scope() {
+    cir.scope {
+    }
+    cir.return
+  }
+// CHECK:  cir.func @empty_scope() {
+// CHECK:    cir.return
+// CHECK:  }
+
+  cir.func @scope_with_return() -> !cir.int<u, 32> {
+    %0 = cir.alloca !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>, ["__retval"] {alignment = 4 : i64}
+    cir.scope {
+      %2 = cir.const #cir.int<0> : !cir.int<u, 32>
+      cir.store %2, %0 : !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>
+      %3 = cir.load %0 : !cir.ptr<!cir.int<u, 32>>, !cir.int<u, 32>
+      cir.return %3 : !cir.int<u, 32>
+    }
+    %1 = cir.load %0 : !cir.ptr<!cir.int<u, 32>>, !cir.int<u, 32>
+    cir.return %1 : !cir.int<u, 32>
+  }
+
+// CHECK:  cir.func @scope_with_return() -> !cir.int<u, 32> {
+// CHECK:    %0 = cir.alloca !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>, ["__retval"] {alignment = 4 : i64}
+// CHECK:    cir.br ^bb1
+// CHECK:  ^bb1:  // pred: ^bb0
+// CHECK:    %1 = cir.const #cir.int<0> : !cir.int<u, 32>
+// CHECK:    cir.store %1, %0 : !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>
+// CHECK:    %2 = cir.load %0 : !cir.ptr<!cir.int<u, 32>>, !cir.int<u, 32>
+// CHECK:    cir.return %2 : !cir.int<u, 32>
+// CHECK:  ^bb2:  // no predecessors
+// CHECK:    %3 = cir.load %0 : !cir.ptr<!cir.int<u, 32>>, !cir.int<u, 32>
+// CHECK:    cir.return %3 : !cir.int<u, 32>
+// CHECK:  }
+
+}
diff --git a/clang/tools/cir-opt/CMakeLists.txt b/clang/tools/cir-opt/CMakeLists.txt
index ca7ee44f6fd75..cae7de6f056a9 100644
--- a/clang/tools/cir-opt/CMakeLists.txt
+++ b/clang/tools/cir-opt/CMakeLists.txt
@@ -24,6 +24,7 @@ clang_target_link_libraries(cir-opt
   clangCIR
   clangCIRLoweringDirectToLLVM
   MLIRCIR
+  MLIRCIRTransforms
 )
 
 target_link_libraries(cir-opt
diff --git a/clang/tools/cir-opt/cir-opt.cpp b/clang/tools/cir-opt/cir-opt.cpp
index 0c0f6dcd9eecf..8e9311c591a3e 100644
--- a/clang/tools/cir-opt/cir-opt.cpp
+++ b/clang/tools/cir-opt/cir-opt.cpp
@@ -22,6 +22,7 @@
 #include "mlir/Pass/PassRegistry.h"
 #include "mlir/Tools/mlir-opt/MlirOptMain.h"
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
+#include "clang/CIR/Dialect/Passes.h"
 #include "clang/CIR/Passes.h"
 
 struct CIRToLLVMPipelineOptions
@@ -39,6 +40,10 @@ int main(int argc, char **argv) {
         cir::direct::populateCIRToLLVMPasses(pm);
       });
 
+  ::mlir::registerPass([]() -> std::unique_ptr<::mlir::Pass> {
+    return mlir::createCIRFlattenCFGPass();
+  });
+
   mlir::registerTransformsPasses();
 
   return mlir::asMainReturnCode(MlirOptMain(

erichkeane

I don't have concerns, but am not the best person whose approval should count here. I have questions/comments on the scope of the flatten pass (that is, what 'while we are here' type stuff it should do), but I'm again not really an authority on these.

erichkeane · 2025-03-12T14:42:39Z

clang/test/CIR/Transforms/scope.cir

+    cir.return
+  }
+// CHECK:  cir.func @foo() {
+// CHECK:    cir.br ^bb1


This here is a touch awkward, right? I know we did a bit of work to remove 'empty' blocks in the pass. I wonder if a future version of this pass should be checking the 'exit' of a block to see if it is a single-out (that is, no decisions being done, just a single br, and merge the two. It seems to fit in well with the flatten.

That is, this function here seems like it should/could be a single block, right?

In the patch I am currently working on I am adding the Canonicalization pass, which merges/removes useless blocks. So this CIR will look better after that is done. (At least I think it will. The canonicalization pass runs before the flattening pass. I can't promise that it will also run after.)

These checks are the same checks that are present in this test in the incubator (except for the fact that we're not abbreviating the int types upstream yet). The test is verifying the flattening pass in isolation, and since the flattening pass doesn't remove unneeded blocks, the output will stay as it is here.

These checks are the same checks that are present in this test in the incubator (except for the fact that we're not abbreviating the int types upstream yet). The test is verifying the flattening pass in isolation, and since the flattening pass doesn't remove unneeded blocks, the output will stay as it is here.

My question was really "should it?" to the "the flattening pass doesn't remove unneeded blocks". But TBH my understanding of what we should expect out of individual passes, or how passes are designed is lacking.

I don't think it should. The general intent is for passes to perform a specific purpose and rely on layering of passes to clean things up. Because the canonicalization pass needs to remove useless blocks anyway, there is no advantage to also doing that in the flatten pass.

Yea, removing blocks should be on a separated pass. However, if during some transformation in the flattening it's convenient/easy to delete certain blocks without extra work, we should just do it (pretty common all around), but it's really on a case basis!

erichkeane · 2025-03-12T14:44:11Z

clang/test/CIR/Transforms/scope.cir

+
+// CHECK:  cir.func @scope_with_return() -> !cir.int<u, 32> {
+// CHECK:    %0 = cir.alloca !cir.int<u, 32>, !cir.ptr<!cir.int<u, 32>>, ["__retval"] {alignment = 4 : i64}
+// CHECK:    cir.br ^bb1


This is another block that it seems to me could be removed/merged with bb1.

I guess there are later passes that do these sorts of merges? Also, removing bb2 (which is an obviously dead block).

I guess there are later passes that do these sorts of merges?

yes!

andykaylor · 2025-03-12T20:22:15Z

@bcardosolopes Do you have any comments on this before I merge it?

bcardosolopes

LGTM

A previous change added the cir-flatten-cfg transform and tested it by lowering a function with nested scopes to LLVM IR. This change adds support for invoking the cir-flatten-cfg pass from the cir-opt tool and adds a new test to verify that functionality in isolation.

andykaylor requested review from bcardosolopes, erichkeane, lanza and dkolsen-pgi March 11, 2025 23:23

llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Mar 11, 2025

erichkeane approved these changes Mar 12, 2025

View reviewed changes

bcardosolopes approved these changes Mar 12, 2025

View reviewed changes

andykaylor merged commit be0215d into llvm:main Mar 12, 2025
14 checks passed

andykaylor deleted the cir-flatten-test branch April 10, 2025 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CIR] Add transform test for cir-flatten-cfg #130861

[CIR] Add transform test for cir-flatten-cfg #130861

Uh oh!

andykaylor commented Mar 11, 2025

Uh oh!

llvmbot commented Mar 11, 2025 •

edited

Loading

Uh oh!

erichkeane left a comment

Uh oh!

erichkeane Mar 12, 2025

Uh oh!

dkolsen-pgi Mar 12, 2025

Uh oh!

andykaylor Mar 12, 2025

Uh oh!

erichkeane Mar 12, 2025

Uh oh!

andykaylor Mar 12, 2025

Uh oh!

bcardosolopes Mar 12, 2025

Uh oh!

erichkeane Mar 12, 2025

Uh oh!

bcardosolopes Mar 12, 2025

Uh oh!

andykaylor commented Mar 12, 2025

Uh oh!

bcardosolopes left a comment

Uh oh!

Uh oh!

Uh oh!

[CIR] Add transform test for cir-flatten-cfg #130861

[CIR] Add transform test for cir-flatten-cfg #130861

Uh oh!

Conversation

andykaylor commented Mar 11, 2025

Uh oh!

llvmbot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andykaylor commented Mar 12, 2025

Uh oh!

bcardosolopes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Mar 11, 2025 •

edited

Loading