[CIR] Add 'core-flat' as an option value for '-emit-mlir=' #1466

darkbuck · 2025-03-11T06:41:30Z

Both 'func' and 'scf' have structure control flow constraints, e.g., 'func.return' needs to have 'func.func' parent op. The alternative approach to lower CIR into MLIR standard dialects is to lower CIR into 'cf' dialect and perform structurization on all or selected regions.
Add 'core-flat' option to enable CFG flattening when lowering CIR into MLIR standard dialects.
Add 'cir-unify-func-return' pass to unify returns into branches to a trialing block dedicated for function return.
Fix 'cir.br' and 'cir.return' lowering to MLIR and allow function declarations.

bcardosolopes

Thanks for improving things here.

Add 'core-flat' option to enable CFG flattening when lowering CIR into MLIR standard dialects.

Instead of adding a new core-flat mode, you should instead use the information on whether we are in the “throughMLIR” pipeline and change how CFGFlattening works based on that (e.g. we could change which rewrites will run for direct LLVM or through MLIR)

Add 'cir-unify-func-return' pass to unify returns into branches to a trialing block dedicated for function return.

This should be an additional rewrites inside the existing flattening pass.

bcardosolopes · 2025-03-13T18:50:14Z

(cc @keryell)

darkbuck · 2025-03-13T19:01:22Z

Thanks for improving things here.

Add 'core-flat' option to enable CFG flattening when lowering CIR into MLIR standard dialects.

Instead of adding a new core-flat mode, you should instead use the information on whether we are in the “throughMLIR” pipeline and change how CFGFlattening works based on that (e.g. we could change which rewrites will run for direct LLVM or through MLIR)

Let me check what's we should follow.

Add 'cir-unify-func-return' pass to unify returns into branches to a trialing block dedicated for function return.

This should be an additional rewrites inside the existing flattening pass.

here's my understanding. flatten could be run for either ThroughMLIR or DirectToLLVM options. For the later, return don't need unifiying that as 'llvm.return' doesn't have the restriction that its parent must be the function operaton. Only 'func.return' has that restriction. If we want to represent any C/C++ code in the 'func' dialect, we need to unify return. It seems to me that unifying should be dedicated pass or at least one option depending which dialect we want to lower CIR to. I though it'd better to separate them. Any suggestion?

bcardosolopes · 2025-03-13T22:09:56Z

here's my understanding. flatten could be run for either ThroughMLIR or DirectToLLVM options. For the later, return don't need unifiying that as 'llvm.return' doesn't have the restriction that its parent must be the function operaton. Only 'func.return' has that restriction. If we want to represent any C/C++ code in the 'func' dialect, we need to unify return. It seems to me that unifying should be dedicated pass or at least one option depending which dialect we want to lower CIR to. I though it'd better to separate them. Any suggestion?

I'm basically suggesting:

void FlattenCFGPass::runOnOperation() {
  RewritePatternSet patterns(&getContext());
  populateFlattenCFGPatterns(patterns);

  // Collect operations to apply patterns.
  llvm::SmallVector<Operation *, 16> ops;
  getOperation()->walk<mlir::WalkOrder::PostOrder>([&](Operation *op) {
    if (isa<IfOp, ScopeOp, SwitchOp, LoopOpInterface, TernaryOp, TryOp>(op))
      ops.push_back(op);
  });

  // Apply patterns.
  if (applyOpPatternsGreedily(ops, std::move(patterns)).failed())
    signalPassFailure();

  // YOUR THING GOES HERE.
  if (IsThroughMLIRpath) {
    unifyReturn(...) // ideally you should just use walkers on rewritters
  }
}

I don't see why you need an optional extra compiler flag for doing this if it always needs to be enabled when lowering to core dialects.

darkbuck · 2025-03-14T17:08:41Z

here's my understanding. flatten could be run for either ThroughMLIR or DirectToLLVM options. For the later, return don't need unifiying that as 'llvm.return' doesn't have the restriction that its parent must be the function operaton. Only 'func.return' has that restriction. If we want to represent any C/C++ code in the 'func' dialect, we need to unify return. It seems to me that unifying should be dedicated pass or at least one option depending which dialect we want to lower CIR to. I though it'd better to separate them. Any suggestion?

I'm basically suggesting:
void FlattenCFGPass::runOnOperation() {
  RewritePatternSet patterns(&getContext());
  populateFlattenCFGPatterns(patterns);

  // Collect operations to apply patterns.
  llvm::SmallVector<Operation *, 16> ops;
  getOperation()->walk<mlir::WalkOrder::PostOrder>([&](Operation *op) {
    if (isa<IfOp, ScopeOp, SwitchOp, LoopOpInterface, TernaryOp, TryOp>(op))
      ops.push_back(op);
  });

  // Apply patterns.
  if (applyOpPatternsGreedily(ops, std::move(patterns)).failed())
    signalPassFailure();

  // YOUR THING GOES HERE.
  if (IsThroughMLIRpath) {
    unifyReturn(...) // ideally you should just use walkers on rewritters
  }
}
I don't see why you need an optional extra compiler flag for doing this if it always needs to be enabled when lowering to core dialects.

Fix that in the new revision. Add a new 'through-mlir' option in 'cir-flatten-cfg' pass to unify returns if we lower CIR through MLIR.

However, after 49f7c80, we don't has a dedicated -emit-cir-flat option. That option is alias to -emit-mlir=cir-flat now. Besides specifying the flattening action, the later will skip the lowering from CIR to MLIR. It seems to me we needs 'core-flat' to run both flattening and lowering-to-MLIR together.

keryell

Great!
Just a few remarks and questions.

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp

keryell · 2025-03-14T22:19:17Z

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp

+  unsigned numReturnOps = 0;
+  func->walk([&](cir::ReturnOp op) {
+    ops.push_back(op.getOperation());
+    ++numReturnOps;


Just use ops.size()?

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp

keryell · 2025-03-14T22:51:14Z

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp

@@ -937,4 +1015,10 @@ std::unique_ptr<Pass> createFlattenCFGPass() {
  return std::make_unique<FlattenCFGPass>();
 }

+std::unique_ptr<Pass> createFlattenCFGPass(bool throughMLIR) {


Could this function just be the previous one extended with a defaulted parameter, such as

Suggested change

std::unique_ptr<Pass> createFlattenCFGPass(bool throughMLIR) {

std::unique_ptr<Pass> createFlattenCFGPass(bool throughMLIR = false) {

or true?

darkbuck · 2025-03-15T03:27:46Z

Great! Just a few remarks and questions.

Thanks for your valuable comments. I fixed all of them except the last one. It's confusing to me whether we don't use the default constructor for each pass. I understand that passes like LifetimeCheck have special parameters not handled yet by the tablegen. But, there are passes not taking that unique parameter. Why do we still customize their constructors?

bcardosolopes · 2025-03-17T17:24:37Z

That option is alias to -emit-mlir=cir-flat now. Besides specifying the flattening action, the later will skip the lowering from CIR to MLIR. It seems to me we needs 'core-flat' to run both flattening and lowering-to-MLIR together.

I don't yet see why this flag is needed, if you want to test MLIR output, you'd use -emit-mlir=core, if you want to test the flattened CIR, you need to use -emit-mlir=cir-flat. Your test could have two different RUN lines, or alternatively you can use print-before/print-after knobs to test more fine grained steps if you wish to do so

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp

…#1081) Now implement the same as [OG](https://github.com/llvm/clangir/blob/7619b20d7461b2d46c17a3154ec4b2f12ca35ea5/clang/lib/CodeGen/CGBuiltin.cpp#L7886), which is to call llvm aarch64 intrinsic which would eventually become [an ARM64 instruction](https://developer.arm.com/documentation/ddi0596/2021-03/SIMD-FP-Instructions/ABS--Absolute-value--vector--?lang=en). However, clearly there is an alternative, which is to extend CIR::AbsOp and CIR::FAbsOp to support vector type and only lower it at LLVM Lowering stage to either [LLVM::FAbsOP ](https://mlir.llvm.org/docs/Dialects/LLVM/#llvmintrfabs-llvmfabsop) or [[LLVM::AbsOP ]](https://mlir.llvm.org/docs/Dialects/LLVM/#llvmintrabs-llvmabsop), provided LLVM dialect could do the right thing of TargetLowering by translating to llvm aarch64 intrinsic eventually. The question is whether it is worth doing it? Any way, put up this diff for suggestions and ideas.

…f -fstrict-vtable-pointers (llvm#1138) Without using flag `-fstrict-vtable-pointers`, `__builtin_launder` is a noop. This PR implements that, and leave implementation for the case of `-fstrict-vtable-pointers` to future where there is a need. This PR also adapted most of test cases from [OG test case](https://github.com/llvm/clangir/blob/3aed38cf52e72cb51a907fad9dd53802f6505b81/clang/test/CodeGenCXX/builtin-launder.cpp#L1). I didn't use test cases in the namespace [pessimizing_cases](https://github.com/llvm/clangir/blob/3aed38cf52e72cb51a907fad9dd53802f6505b81/clang/test/CodeGenCXX/builtin-launder.cpp#L269), as they have no difference even when `-fstrict-vtable-pointers` is on.

…1165) If a record type contains an array of non-record types, we can generate a copy for it inside the copy constructor, as CodeGen does. CodeGen does so for arrays of record types where applicable as well, but we'll want to represent the construction of those explicitly, as outlined in llvm#1055.

… semantics of C++ initializer list (llvm#1121) I don't finish all work about `cir.initlist`. But I want to get some feedback about the cir design to make sure I am in correct way. Fixed: llvm#777

This PR also changed implementation of BI__builtin_neon_vshlq_v into using CIR ShiftOp

After I rebased, I found these problems with Spec2017. I was surprised why it doesn't have problems. Maybe some updates in LLVM part.

The requirement for the size of then-else part of cir.ternary operation seems to be too conservative. Like the example shows, it is possible the regions got expanded during the transformation.

…m#1169) For example, the following reaches ["NYI"](https://github.com/llvm/clangir/blob/c8b626d49e7f306052b2e6d3ce60b1f689d37cb5/clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp#L348) when lowering to AArch64: ``` typedef struct { union { struct { char a, b; }; char c; }; } A; void foo(A a) {} void bar() { A a; foo(a); } ``` Currently, the value of the struct becomes a bitcast operation, so we can simply extend `findAlloca` to be able to trace the source alloca properly, then use that for the [coercion](https://github.com/llvm/clangir/blob/c8b626d49e7f306052b2e6d3ce60b1f689d37cb5/clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp#L341) through memory. I have also added a test for this case.

Added a few FIXMEs. There are 2 types of FIXMEs; 1. Most of them are missing func call and parameter attributes. I didn't add for all missing sites for this type as it would have been just copy pastes. 2. FIXME in lambda __invoke(): OG simply returns but CIR generates call to llvm.trap. This is just temporary and we will fix in in near future. But I feel I should still list those IRs so once we fix problem with codegen of invoke, we'd get test failure on this one and fix it. Actually, this way, this test file would be a natural test case for implementation of invoke.

There are scenarios where we are not emitting cleanups, this commit starts to pave the way to be more complete in that area. Small addition of skeleton here plus some fixes. Both `clang/test/CIR/CodeGen/vla.c` and `clang/test/CIR/CodeGen/nrvo.cpp` now pass in face of this code path.

llvm#1145) Fix llvm#793

…lvm#1166) Close llvm#1131 This is another solution to llvm#1160 This patch revert llvm#1007 and remain its test. The problem described in llvm#1007 is workaround by skipping the check of equivalent of element types in arrays. We can't mock such checks simply by adding another attribute to `ConstStructAttr` since the types are aggregated. e.g., we have to handle the cases like `struct { union { ... } }` and `struct { struct { union { ... } } }` and so on. To make it, we have to introduce what I called "two type systems" in llvm#1160. This is not very good giving it removes a reasonable check. But it might not be so problematic since the Sema part has already checked it. (Of course, we still need face the risks to introduce new bugs any way)

…m#1173)

…of floating type (llvm#1174) [PR1132](llvm#1132) implements missing feature `fpUnaryOPsSupportVectorType`, so revisit this code. One another thing changed is that I stopped using `cir::isAnyFloatingPointType` as it contains types like long double and FP80 which are not supported by the [builtin's signature](https://clang.llvm.org/docs/LanguageExtensions.html#vector-builtins)

[OG's implementation ](https://github.com/llvm/clangir/blob/aaf38b30d31251f3411790820c5e1bf914393ddc/clang/lib/CodeGen/CGBuiltin.cpp#L7527) provides one common code to handle all neon SISD intrinsics. But IMHO, it entangles different things together which hurts readability. Here, We start with simple easy-to-understand approach with specific case. And in the future, as we handle more intrinsics, we may come up with a few simple common patterns.

Lower neon vabsd_s64

…lvm#1431) Implements `::verify` for operations cir.atomic.xchg and cir.atomic.cmp_xchg I believe the existing regression tests don't get to the CIR level type check failure and I was not able to implement a case that does. Most attempts of reproducing cir.atomic.xchg type check failure were along the lines of: ``` int a; long long b,c; __atomic_exchange(&a, &b, &c, memory_order_seq_cst); ``` And they seem to never trigger the failure on `::verify` because they fail earlier in function parameter checking: ``` exmp.cpp:7:27: error: cannot initialize a parameter of type 'int *' with an rvalue of type 'long long *' 7 | __atomic_exchange(&a, &b, &c, memory_order_seq_cst); | ^~ ``` Closes llvm#1378 .

Lower neon vcaled_f64

This PR adds a new boolean flag to the `cir.load` and the `cir.store` operation that distinguishes nontemporal loads and stores. Besides, this PR also adds support for the `__builtin_nontemporal_load` and the `__builtin_nontemporal_store` intrinsic function.

- Both 'func' and 'scf' have structure control flow constraints, e.g., 'func.return' needs to have 'func.func' parent op. The alternative approach to lower CIR into MLIR standard dialects is to lower CIR into 'cf' dialect and then perform structurization on all or selected regions. - Add 'core-flat' option to enable CFG flattening when lowering CIR into MLIR standard dialects. - Enhance 'cir-flatten-cfg' pass to unify returns into branches to a dedicated return block. - Fix 'cir.br' and 'cir.return' lowering to MLIR and allow function declarations.

darkbuck · 2025-03-18T12:25:49Z

That option is alias to -emit-mlir=cir-flat now. Besides specifying the flattening action, the later will skip the lowering from CIR to MLIR. It seems to me we needs 'core-flat' to run both flattening and lowering-to-MLIR together.

I don't yet see why this flag is needed, if you want to test MLIR output, you'd use -emit-mlir=core, if you want to test the flattened CIR, you need to use -emit-mlir=cir-flat. Your test could have two different RUN lines, or alternatively you can use print-before/print-after knobs to test more fine grained steps if you wish to do so

By specifying -emit-mlir=cir-flat, it implies the program will be lowered from CIR to LLVM directly. However, we need a a flatten CIR to lower a program into MLIR.

bcardosolopes · 2025-03-19T01:51:17Z

By specifying -emit-mlir=cir-flat, it implies the program will be lowered from CIR to LLVM directly. However, we need a a flatten CIR to lower a program into MLIR.

Some confusion might be going on: there should not be a new flag coordinating this, you have to use both -fno-clangir-direct-lowering and -emit-mlir=cir-flat if your flat cir wants to be taught the extra pass.

darkbuck · 2025-03-19T02:15:36Z

By specifying -emit-mlir=cir-flat, it implies the program will be lowered from CIR to LLVM directly. However, we need a a flatten CIR to lower a program into MLIR.

Some confusion might be going on: there should not be a new flag coordinating this, you have to use both -fno-clangir-direct-lowering and -emit-mlir=cir-flat if your flat cir wants to be taught the extra pass.

That combination could generate CIR output as -emit-mlir=cir-flat CIR output only.

281
282     switch (action) {
283     case CIRGenAction::OutputType::EmitMLIR: {
284       switch (feOptions.MLIRTargetDialect) {
285       case clang::frontend::MLIR_CORE:
286         // case for direct lowering is already checked in compiler invocation
287         // no need to check here
288         emitMLIR(lowerFromCIRToMLIR(mlirMod, mlirCtx.get()), false);
289         break;
290       case clang::frontend::MLIR_LLVM: {
291         mlir::ModuleOp loweredMLIRModule =
292             feOptions.ClangIRDirectLowering
293                 ? direct::lowerDirectlyFromCIRToLLVMDialect(mlirMod)
294                 : lowerFromCIRToMLIRToLLVMDialect(mlirMod, mlirCtx.get());
295         emitMLIR(loweredMLIRModule, false);
296         break;
297       }
298       case clang::frontend::MLIR_CIR:
299       case clang::frontend::MLIR_CIR_FLAT:
300         emitMLIR(mlirMod, feOptions.ClangIRDisableCIRVerifier);
301         break;
302       }
303       break;
304     }

Once -emit-mlir=cir-flat is specified, we just stop after CIR-to-CIR passes and won't be able to run into the CIR-to-MLIR conversion (Line 298 to Line 300). core-flat is introduced to enable CFG flattening and later CFG structurization on the path from CIR to MLIR.

bcardosolopes · 2025-03-19T02:20:49Z

Once -emit-mlir=cir-flat is specified, we just stop after CIR-to-CIR passes and won't be able to run into the CIR-to-MLIR conversion (Line 298 to Line 300). core-flat is introduced to enable CFG flattening and later CFG structurization on the path from CIR to MLIR.

I don't think we need the extra flag, we have the necessary combinations already. If you want to test CIR-to-MLIR conversion add the extra RUN lines as I previous explained.

darkbuck · 2025-03-19T02:22:53Z

Once -emit-mlir=cir-flat is specified, we just stop after CIR-to-CIR passes and won't be able to run into the CIR-to-MLIR conversion (Line 298 to Line 300). core-flat is introduced to enable CFG flattening and later CFG structurization on the path from CIR to MLIR.

I don't think we need the extra flag, we have the necessary combinations already. If you want to test CIR-to-MLIR conversion add the extra RUN lines as I previous explained.

For testing, that extra RUN is good enough. But, we want to clang option(s) to generate MLIR directly from C/C++ source. Any suggestion?

bcardosolopes · 2025-03-19T02:28:17Z

we want to clang option(s) to generate MLIR directly from C/C++ source

This option is -fno-clangir-direct-lowering, there won't be two modes of it, it should always change cfg flatten when it is passed.

darkbuck · 2025-03-19T02:37:11Z

we want to clang option(s) to generate MLIR directly from C/C++ source

This option is -fno-clangir-direct-lowering, there won't be two modes of it, it should always change cfg flatten when it is passed.

Could we remove 'cir-flat' and add back the original emit-cir-flat? After that, we have 3 kinds of outpur IR: CORE, LLVM, and CIR. -emit-cir-flat controls whether the CIR needs flatting during CIR-to-CIR passes and -fno-clangir-direct-lowering specifies the alternative approach (CIR-to-LLVM vs CIR-to-MLIR-to-LLVM) for the LLVM dialect output. Does that sound reasonable?

bcardosolopes · 2025-03-20T18:37:04Z

Could we remove 'cir-flat' and add back the original emit-cir-flat?

I'm surprised the behavior changed, I was expecting -emit-mlir=cir-flat to be equivalent to -emit-cir-flat. If not, it sounds like a bug? (cc: @Jezurko)

Jezurko · 2025-03-24T08:54:59Z

Hello,

I am not 100 % sure if I understand this conversation correctly, so correct me if I'm wrong. The goal is to perform the flattening pass before applying the passes that lower CIR to Core MLIR?

I believe there is some misunderstanding on how the flags worked/work now:

you have to use both -fno-clangir-direct-lowering and -emit-mlir=cir-flat if your flat cir wants to be taught the extra pass.

No, this combination would stop after emitting (flattened) CIR, it would not perform further passes. That should be inline with the behaviour before the changes. I checked out a commit before my change and the behaviour seems to be the same for this combination of flags (replacing -emit-mlir=cir-flat with -emit-cir-flat), unless I'm missing something.

-emit-cir-flat controls whether the CIR needs flatting during CIR-to-CIR passes

I don't believe that's true. The -emit-cir-flat flag was an action that was used to emit flattened CIR. Here is the deleted code that used to do that.

I'm surprised the behavior changed, I was expecting -emit-mlir=cir-flat to be equivalent to -emit-cir-flat. If not, it sounds like a bug?

If that's the case, I'm happy to work on fixing it.

@darkbuck In what way did you use the -emit-cir-flat option before the change? Could you provide the list of flags and the expected behaviour/output? I might be missing something, so maybe it could clear up some confusion on my side.

bcardosolopes · 2025-03-24T18:04:32Z

I believe there is some misunderstanding on how the flags worked/work now:

This is my takeaway from this discussion so far

If that's the case, I'm happy to work on fixing it.

Thanks for clarifying your change didn't change it!

darkbuck · 2025-03-25T04:30:45Z

Hello,

I am not 100 % sure if I understand this conversation correctly, so correct me if I'm wrong. The goal is to perform the flattening pass before applying the passes that lower CIR to Core MLIR?

I believe there is some misunderstanding on how the flags worked/work now:

you have to use both -fno-clangir-direct-lowering and -emit-mlir=cir-flat if your flat cir wants to be taught the extra pass.

No, this combination would stop after emitting (flattened) CIR, it would not perform further passes. That should be inline with the behaviour before the changes. I checked out a commit before my change and the behaviour seems to be the same for this combination of flags (replacing -emit-mlir=cir-flat with -emit-cir-flat), unless I'm missing something.

-emit-cir-flat controls whether the CIR needs flatting during CIR-to-CIR passes

I don't believe that's true. The -emit-cir-flat flag was an action that was used to emit flattened CIR. Here is the deleted code that used to do that.

I'm surprised the behavior changed, I was expecting -emit-mlir=cir-flat to be equivalent to -emit-cir-flat. If not, it sounds like a bug?

If that's the case, I'm happy to work on fixing it.

@darkbuck In what way did you use the -emit-cir-flat option before the change? Could you provide the list of flags and the expected behaviour/output? I might be missing something, so maybe it could clear up some confusion on my side.

Extracted include/clang/Driver/Options.td

3079 def emit_cir_flat : Flag<["-"], "emit-cir-flat">, Visibility<[ClangOption, CC1Option]>,
3080   Group<Action_Group>, Alias<emit_mlir_EQ>, AliasArgs<["cir-flat"]>,
3081   HelpText<"Similar to -emit-cir but also lowers structured CFG into basic blocks.">;
3082 /// ClangIR-specific options - END

That's the code I refer to.

Jezurko · 2025-03-25T07:50:15Z

That's the code I refer to.

I see, but the option generated by the non-alias code was still being handled as an action. So my understanding is that the alias should behave the same way as the old option - it is still being handled as an action to emit flattened CIR, only with different scaffolding around it. Could you please explain/show for which combination of flags is the behaviour different compared to the old version?

Jezurko · 2025-03-25T08:22:53Z

Looking more into the PR I would like to check if I understand the goals of the newly added code correctly. You are adding a new path to the lowering process, that works on flattened CIR and goes through MLIR Core dialects, is that right?
In that case I would argue that adding the core-flat value makes some sense, as it is a new path (from what I see, the current lowering through Core MLIR doesn't flatten CIR before conversions).

Another option (that quickly comes to my mind) would be to remove both the cir-flat and core-flat values and add a bool flag - something along the path -mlir-flatten-cfg. Is this maybe what you had in mind when suggesting reverting to -emit-cir-flat?

Please, correct me if I'm wrong

darkbuck · 2025-03-25T12:44:45Z

Looking more into the PR I would like to check if I understand the goals of the newly added code correctly. You are adding a new path to the lowering process, that works on flattened CIR and goes through MLIR Core dialects, is that right? In that case I would argue that adding the core-flat value makes some sense, as it is a new path (from what I see, the current lowering through Core MLIR doesn't flatten CIR before conversions).

Another option (that quickly comes to my mind) would be to remove both the cir-flat and core-flat values and add a bool flag - something along the path -mlir-flatten-cfg. Is this maybe what you had in mind when suggesting reverting to -emit-cir-flat?

Please, correct me if I'm wrong

Previously, to generate LLVM, we had two options: one was directly lowered into LLVM dialect (direct-CIR-to-LLVM) , and the other one was through MLIR first, followed by MLIR to LLVM (CIR-to-MLIR-to-LLVM). Flattening is just an option to prepare the CIR before lowering it. That CIR would be fed into either approach (direct-CIR-to-LLVM, or CIR-to-MLIR-to-LLVM). In that sense, core-flat should be removed and recover the original -emit-flat-cir option. That -emit-flat-cir is only used to flat CIR or not (before the loweirng.)

bcardosolopes · 2025-03-25T18:03:04Z

In that case I would argue that adding the core-flat value makes some sense, as it is a new path (from what I see, the current lowering through Core MLIR doesn't flatten CIR before conversions).

My previous point is that through core path is still experimental and we don't need two paths for it, it should always do the flattening this PR implements, therefore no new flags needed.

darkbuck · 2025-03-25T22:36:19Z

In that case I would argue that adding the core-flat value makes some sense, as it is a new path (from what I see, the current lowering through Core MLIR doesn't flatten CIR before conversions).

My previous point is that through core path is still experimental and we don't need two paths for it, it should always do the flattening this PR implements, therefore no new flags needed.

Do you mean we turn on flattening by default?

Jezurko · 2025-03-26T09:01:56Z

My previous point is that through core path is still experimental and we don't need two paths for it, it should always do the flattening this PR implements, therefore no new flags needed.

I see. Yes, in that case it should be enough to just modify the expression assigned to bool flattenCIR accordingly.

recover the original -emit-flat-cir option. That -emit-flat-cir is only used to flat CIR or not (before the loweirng.)

That was not the old behaviour of the flag. The behaviour did not change. The flag was for emitting (~printing) of CIR after flattening. If you look into the descriptions/help texts of -emit-cir and -emit-cir-flat you will see that the purpose of that flag is to emit a .cir file that contains the program represented in structured/flattened CIR. That purpose did not change.

darkbuck · 2025-03-26T20:24:48Z

My previous point is that through core path is still experimental and we don't need two paths for it, it should always do the flattening this PR implements, therefore no new flags needed.

I see. Yes, in that case it should be enough to just modify the expression assigned to bool flattenCIR accordingly.

recover the original -emit-flat-cir option. That -emit-flat-cir is only used to flat CIR or not (before the loweirng.)

That was not the old behaviour of the flag. The behaviour did not change. The flag was for emitting (~printing) of CIR after flattening. If you look into the descriptions/help texts of -emit-cir and -emit-cir-flat you will see that the purpose of that flag is to emit a .cir file that contains the program represented in structured/flattened CIR. That purpose did not change.

bcardosolopes · 2025-03-26T23:34:38Z

Do you mean we turn on flattening by default?

Yes for the lowering path from CIR to Core, of course it should only run the passes that make sense for Core (and not the other ones that run for Direct, but I already gave this feedback as part of the review). Note that I'm assuming that what you are doing here is useful, and none of the other folks interested in core lowering seems to have issue with this, that's why I'm saying we don't need two paths for core.

darkbuck requested review from lanza and bcardosolopes as code owners March 11, 2025 06:41

darkbuck force-pushed the hliao/main/emit-core-flat-review branch from 29502b8 to 49a2b45 Compare March 12, 2025 13:32

bcardosolopes requested changes Mar 13, 2025

View reviewed changes

darkbuck force-pushed the hliao/main/emit-core-flat-review branch from 49a2b45 to 6fd8f88 Compare March 14, 2025 17:00

keryell requested changes Mar 14, 2025

View reviewed changes

darkbuck force-pushed the hliao/main/emit-core-flat-review branch from 6fd8f88 to 64971e2 Compare March 15, 2025 03:23

bcardosolopes requested changes Mar 17, 2025

View reviewed changes

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp Outdated Show resolved Hide resolved

clang/lib/CIR/Dialect/Transforms/FlattenCFG.cpp Outdated Show resolved Hide resolved

ghehg and others added 16 commits March 17, 2025 13:53

[CIR][Dialect] Introduce StdInitializerListOp to represent high-level…

ee4f0e1

… semantics of C++ initializer list (llvm#1121) I don't finish all work about `cir.initlist`. But I want to get some feedback about the cir design to make sure I am in correct way. Fixed: llvm#777

[CIR][CIRGen][Builtin][Neon] Lower __builtin_neon_vshl_v (llvm#1134)

89b0cd9

This PR also changed implementation of BI__builtin_neon_vshlq_v into using CIR ShiftOp

[CIR] Correct signedness for createSignedInt (llvm#1167)

286d691

After I rebased, I found these problems with Spec2017. I was surprised why it doesn't have problems. Maybe some updates in LLVM part.

[CIR] Relax the requirement for ternary operation (llvm#1168)

b62258e

The requirement for the size of then-else part of cir.ternary operation seems to be too conservative. Like the example shows, it is possible the regions got expanded during the transformation.

[CIR][NFC] Cleanup warnings post rebase

49e6856

[CIR][Lowering][debuginfo] Disable debug info if -g is not specified (

6c99597

llvm#1145) Fix llvm#793

[CIR][Dialect] Extend BinaryFPToFPBuiltinOp to vector of FP type (llv…

53d7913

…m#1173)

AmrDeveloper and others added 5 commits March 17, 2025 19:20

[CIR][CIRGen][Builtin][Neon] Lower neon vabsd_s64 (llvm#1489)

f2eb0f7

Lower neon vabsd_s64

[CIR][CIRGen][Builtin][Neon] Lower neon vcaled_f64 (llvm#1495)

4cf5576

Lower neon vcaled_f64

[CIR][CIRGen][builtin] handle _mm_pause (llvm#1493)

7679ba9

lanza force-pushed the main branch from 0364dd2 to 79d0d74 Compare March 18, 2025 02:22

darkbuck force-pushed the hliao/main/emit-core-flat-review branch from 64971e2 to 8ec33f0 Compare March 18, 2025 06:21

darkbuck closed this Mar 26, 2025

darkbuck reopened this Mar 26, 2025

lanza force-pushed the main branch from d3fe299 to 98e8811 Compare April 9, 2025 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR] Add 'core-flat' as an option value for '-emit-mlir=' #1466

[CIR] Add 'core-flat' as an option value for '-emit-mlir=' #1466

darkbuck commented Mar 11, 2025

bcardosolopes left a comment •

edited

Loading

bcardosolopes commented Mar 13, 2025

darkbuck commented Mar 13, 2025

bcardosolopes commented Mar 13, 2025

darkbuck commented Mar 14, 2025 •

edited

Loading

keryell left a comment

keryell Mar 14, 2025

keryell Mar 14, 2025

darkbuck commented Mar 15, 2025

bcardosolopes commented Mar 17, 2025

darkbuck commented Mar 18, 2025

bcardosolopes commented Mar 19, 2025

darkbuck commented Mar 19, 2025 •

edited

Loading

bcardosolopes commented Mar 19, 2025

darkbuck commented Mar 19, 2025

bcardosolopes commented Mar 19, 2025 •

edited

Loading

darkbuck commented Mar 19, 2025

bcardosolopes commented Mar 20, 2025 •

edited

Loading

Jezurko commented Mar 24, 2025 •

edited

Loading

bcardosolopes commented Mar 24, 2025

darkbuck commented Mar 25, 2025

Jezurko commented Mar 25, 2025

Jezurko commented Mar 25, 2025 •

edited

Loading

darkbuck commented Mar 25, 2025

bcardosolopes commented Mar 25, 2025 •

edited

Loading

darkbuck commented Mar 25, 2025

Jezurko commented Mar 26, 2025

darkbuck commented Mar 26, 2025

bcardosolopes commented Mar 26, 2025 •

edited

Loading

	std::unique_ptr<Pass> createFlattenCFGPass(bool throughMLIR) {
	std::unique_ptr<Pass> createFlattenCFGPass(bool throughMLIR = false) {

[CIR] Add 'core-flat' as an option value for '-emit-mlir=' #1466

Are you sure you want to change the base?

[CIR] Add 'core-flat' as an option value for '-emit-mlir=' #1466

Conversation

darkbuck commented Mar 11, 2025

bcardosolopes left a comment • edited Loading

Choose a reason for hiding this comment

bcardosolopes commented Mar 13, 2025

darkbuck commented Mar 13, 2025

bcardosolopes commented Mar 13, 2025

darkbuck commented Mar 14, 2025 • edited Loading

keryell left a comment

Choose a reason for hiding this comment

keryell Mar 14, 2025

Choose a reason for hiding this comment

keryell Mar 14, 2025

Choose a reason for hiding this comment

darkbuck commented Mar 15, 2025

bcardosolopes commented Mar 17, 2025

darkbuck commented Mar 18, 2025

bcardosolopes commented Mar 19, 2025

darkbuck commented Mar 19, 2025 • edited Loading

bcardosolopes commented Mar 19, 2025

darkbuck commented Mar 19, 2025

bcardosolopes commented Mar 19, 2025 • edited Loading

darkbuck commented Mar 19, 2025

bcardosolopes commented Mar 20, 2025 • edited Loading

Jezurko commented Mar 24, 2025 • edited Loading

bcardosolopes commented Mar 24, 2025

darkbuck commented Mar 25, 2025

Jezurko commented Mar 25, 2025

Jezurko commented Mar 25, 2025 • edited Loading

darkbuck commented Mar 25, 2025

bcardosolopes commented Mar 25, 2025 • edited Loading

darkbuck commented Mar 25, 2025

Jezurko commented Mar 26, 2025

darkbuck commented Mar 26, 2025

bcardosolopes commented Mar 26, 2025 • edited Loading

bcardosolopes left a comment •

edited

Loading

darkbuck commented Mar 14, 2025 •

edited

Loading

darkbuck commented Mar 19, 2025 •

edited

Loading

bcardosolopes commented Mar 19, 2025 •

edited

Loading

bcardosolopes commented Mar 20, 2025 •

edited

Loading

Jezurko commented Mar 24, 2025 •

edited

Loading

Jezurko commented Mar 25, 2025 •

edited

Loading

bcardosolopes commented Mar 25, 2025 •

edited

Loading

bcardosolopes commented Mar 26, 2025 •

edited

Loading