Skip to content
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.

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jezurko
Copy link

@Jezurko Jezurko commented Feb 7, 2025

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
Copy link
Contributor

xlauko commented Feb 7, 2025

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.

@@ -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>,
Copy link
Member

Choose a reason for hiding this comment

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

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

@bcardosolopes
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants