[CIR] Add option to emit MLIR in LLVM dialect. #1316

Jezurko · 2025-02-07T14:58:43Z

This PR adds the flag -emit-mlir-llvm to allow emitting of MLIR in the LLVM dialect (cc @xlauko who asked me to do this).
I'm not sure if the naming of the flag is the best and maybe someone will have a better idea.
Another solution would be to make the -emit-mlir flag have a value, that specifies the target dialect (CIR/MLIR std dialects/LLVM Dialect).

xlauko · 2025-02-07T15:19:51Z

Instead of the current approach, I recommend using -emit-mlir=std to match the behavior of -emit-mlir, which is ambiguous regarding which MLIR dialect is being referenced. And -emit-mlir=llvm should be used to target the LLVM dialect.

For the case where -emit-mlir=llvm -fclangir-direct-lowering is used, it should directly lower from CIR to LLVM, with -fclangir-direct-lowering remaining an ON by default. And -emit-mlir=llvm -fno-clangir-direct-lowering would proceed through the standard dialects.

bcardosolopes · 2025-02-07T21:05:58Z

clang/include/clang/Driver/Options.td

@@ -3121,6 +3121,8 @@ def emit_cir_flat : Flag<["-"], "emit-cir-flat">, Visibility<[ClangOption, CC1Op
  Group<Action_Group>, HelpText<"Similar to -emit-cir but also lowers structured CFG into basic blocks.">;
 def emit_mlir : Flag<["-"], "emit-mlir">, Visibility<[CC1Option]>, Group<Action_Group>,
  HelpText<"Build ASTs and then lower through ClangIR to MLIR, emit the .milr file">;
+def emit_mlir_llvm : Flag<["-"], "emit-mlir-llvm">, Visibility<[CC1Option]>, Group<Action_Group>,


As you add this flag, might be worth editing the one above to explain that that does to standard dialects instead.

bcardosolopes · 2025-02-07T21:10:30Z

I like @xlauko idea, couple things to add to the discussion:

emit-mlir should be acceptable without additional arguments and default to std for no-direct and llvm for direct.
We need driver tests as part of the PR (check the produced cc1 invocation).
We need a warning if -emit-mlir=std is used with -fclangir-direct-lowering

[CIR] Add option to emit MLIR in a selected dialect This PR adds `-emit-mlir=<std,llvm,cir>` option that emits MLIR in the specified dialect. The `-emit-mlir` option now emits the MLIR in the LLVM dialect when direct lowering to CIR is used (instead of producing an error).

bcardosolopes

Thanks for improving the PR, more comments inline. This still need tests (both driver test and a few simple IR output checks).

clang/include/clang/Driver/Options.td

bcardosolopes · 2025-02-14T15:13:49Z

clang/include/clang/Driver/Options.td

+  HelpText<"Build ASTs and then lower through ClangIR to the selected MLIR dialect, emit the .mlir file. "
+  "Allowed values are `std` for MLIR standard dialects "
+  "`llvm` for the LLVM dialect and `cir` for the ClangIR dialect.">,
+  Values<"std,llvm,cir">,


It's confusing to have CIR here, better remove it. Should be std or LLVM (we already have -emit-cir and this PR is not unifying any of the existing handling either).

bcardosolopes · 2025-02-14T15:16:11Z

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

+        emitCIR();
+        break;
+      }
+      if (loweredMlirModule) {


What happens if this if is not taken?

bcardosolopes · 2025-02-14T15:17:03Z

clang/lib/CIR/FrontendAction/CIRGenAction.cpp

-      flags.enableDebugInfo(/*enable=*/true, /*prettyForm=*/false);
-      loweredMlirModule->print(*outputStream, flags);
+      mlir::ModuleOp loweredMlirModule;
+      switch (feOptions.MLIRTargetDialect) {


After removing CIR support this can be simplified to not use a switch

bcardosolopes · 2025-02-14T15:58:46Z

To expand a bit on testing:

Driver one: should check that -emit-mlir without and without ={std,mlir} will expand to the right one passed down to -cc1. This can be done with -###.
IR one: simple check that the outputed IR is the flavor we promise.

Jezurko · 2025-02-14T16:52:17Z

Driver one: should check that -emit-mlir without and without ={std,mlir} will expand to the right one passed down to -cc1. This can be done with -###.

In the way I've done it -emit-mlir without value does not expand to the -emit-mlir= version. Instead, the value of the MLIRTargetDialect enum stays as MLIR_Default which signalizes that the action handler should decide based on the Direct/Indirect lowering flag (it has to check that flag to decide between direct/indirect lowering to llvm dialect anyway). Should I change that?

Jezurko · 2025-02-19T14:06:42Z

I have updated the PR according to the comments.
I'm not entirely sure if the tests for the driver (via -###) are as envisioned. I'm happy to change them if required

github-actions · 2025-02-19T14:07:54Z

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

bcardosolopes · 2025-02-19T19:04:17Z

clang/test/CIR/emit-mlir.c

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-clangir-direct-lowering -emit-mlir=llvm %s -o - | FileCheck %s -check-prefix=LLVM
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fno-clangir-direct-lowering -emit-mlir=std %s -o - | FileCheck %s -check-prefix=STD
+
+// RUN: %clang -fclangir -Xclang -emit-mlir %s -o - -### 2>&1 | FileCheck %s -check-prefix=OPTS_NO_VALUE


Thanks for the updates! Almost there, few more changes needed (sorry for not catching early):

The version without the EQ should be driver only.

cc1 only accepts the EQ version, requiring either llvm/std.

We can ditch MLIR_Default by picking one of the EQ values at driver time.

andykaylor · 2025-02-20T19:49:09Z

clang/include/clang/Driver/Options.td

+def emit_mlir_EQ : Joined<["-"], "emit-mlir=">, Visibility<[CC1Option]>, Group<Action_Group>,
+  HelpText<"Build ASTs and then lower through ClangIR to the selected MLIR dialect, emit the .mlir file. "
+  "Allowed values are `std` for MLIR standard dialects and `llvm` for the LLVM dialect.">,
+  Values<"std,llvm">,


Can "cir" and "cir-flat" be added here?

I asked to remove it but now in light of upstreaming discussion llvm/llvm-project#127835 (comment), we should probably converge to make upstreaming approach easier. @Jezurko sorry for the noise. We should add cir and cir-flat to match what is likely coming for Flang as part of that discussion. We should also rename std to core.

Andy, should we hold on this until you land to make your life easier or is it enough if @Jezurko applies the changes above?

I think it makes sense to merge this first, then I can update my upstream patch to align with this and the wishes of the reviewers there.

Ok, I will update the list of values and update the action. No worries about the noise :)

@bcardosolopes what about the non-_EQ situation? The current situation here in the incubator is that the -emit-mlir alias to -emit-fir is simply commented out and the CIR variant of -emit-mlir is defined few lines later. Do we want to somehow resolve that in this PR (probably after discussion in the upstream PR)? Or just land the -emit-mlir= option for now and resolve that separately?

I have posted an update in llvm/llvm-project#127835 that uses the -emit-mlir=[cir|core] option. I was referencing this PR when I implemented it, so it shouldn't be too far off. One difference is that I made the MLIR dialect a separate named enum.

Added another small discussion point over there

Jezurko requested review from lanza and bcardosolopes as code owners February 7, 2025 14:58

[CIR] Add option to emit MLIR in LLVM dialect.

3af1d49

Jezurko force-pushed the emit-mlir-llvm branch from 6870e41 to 3af1d49 Compare February 7, 2025 15:10

bcardosolopes reviewed Feb 7, 2025

View reviewed changes

Jezurko force-pushed the emit-mlir-llvm branch from 5d29c0b to bc51bff Compare February 14, 2025 14:59

Jezurko force-pushed the emit-mlir-llvm branch from bc51bff to d51b625 Compare February 14, 2025 15:01

bcardosolopes requested changes Feb 14, 2025

View reviewed changes

Jezurko force-pushed the emit-mlir-llvm branch from b727be6 to 124bede Compare February 19, 2025 14:10

fixup! [CIR] Add option to emit MLIR in LLVM dialect.

47de848

Jezurko force-pushed the emit-mlir-llvm branch from 124bede to 47de848 Compare February 19, 2025 14:12

bcardosolopes requested changes Feb 19, 2025

View reviewed changes

xlauko mentioned this pull request Feb 20, 2025

[CIR] Initial implementation of lowering CIR to MLIR llvm/llvm-project#127835

Open

andykaylor reviewed Feb 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR] Add option to emit MLIR in LLVM dialect. #1316

[CIR] Add option to emit MLIR in LLVM dialect. #1316

Jezurko commented Feb 7, 2025

xlauko commented Feb 7, 2025

bcardosolopes Feb 7, 2025

bcardosolopes commented Feb 7, 2025

bcardosolopes left a comment

bcardosolopes Feb 14, 2025

bcardosolopes Feb 14, 2025

bcardosolopes Feb 14, 2025

bcardosolopes commented Feb 14, 2025

Jezurko commented Feb 14, 2025

Jezurko commented Feb 19, 2025

github-actions bot commented Feb 19, 2025 •

edited

Loading

bcardosolopes Feb 19, 2025

andykaylor Feb 20, 2025

bcardosolopes Feb 20, 2025 •

edited

Loading

andykaylor Feb 20, 2025

Jezurko Feb 21, 2025

andykaylor Feb 21, 2025

bcardosolopes Feb 22, 2025

[CIR] Add option to emit MLIR in LLVM dialect. #1316

Are you sure you want to change the base?

[CIR] Add option to emit MLIR in LLVM dialect. #1316

Conversation

Jezurko commented Feb 7, 2025

xlauko commented Feb 7, 2025

Choose a reason for hiding this comment

bcardosolopes commented Feb 7, 2025

bcardosolopes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes commented Feb 14, 2025

Jezurko commented Feb 14, 2025

Jezurko commented Feb 19, 2025

github-actions bot commented Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 19, 2025 •

edited

Loading

bcardosolopes Feb 20, 2025 •

edited

Loading