Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 12, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions clang/test/CIR/Transforms/scope.cir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

yes!

// 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: }

}
1 change: 1 addition & 0 deletions clang/tools/cir-opt/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ clang_target_link_libraries(cir-opt
clangCIR
clangCIRLoweringDirectToLLVM
MLIRCIR
MLIRCIRTransforms
)

target_link_libraries(cir-opt
Expand Down
5 changes: 5 additions & 0 deletions clang/tools/cir-opt/cir-opt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(
Expand Down