This is an archive of the discontinued LLVM Phabricator instance.

[flang][driver][openmp] Write MLIR for -save-temps

Authored by skatrak on Mar 14 2023, 11:33 AM.



This patch adds support for producing MLIR files when using -save-temps on
flang. One MLIR file will be produced before lowering and optimization passes,
containing the operations produced by the PFT-to-MLIR lowering bridge, and
another at the end of the process, just before LLVM IR generation.

This is accomplished by forwarding the -save-temps flag from the driver to the
frontend, and modifying it to output MLIR files accordingly.

Diff Detail

Event Timeline

skatrak created this revision.Mar 14 2023, 11:33 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
skatrak requested review of this revision.Mar 14 2023, 11:33 AM
This revision is now accepted and ready to land.Mar 15 2023, 9:49 AM

Please add tests ;)

tschuett added inline comments.

static is cuter.

What is the policy on trivial braces in the frontend?

What is the policy on trivial braces in the frontend?

What do you mean by the frontend? Different people have different understanding of what "the frontend" is.

I try to follow

Frontend is in the file name.


useless braces


useless braces


useless braces

skatrak updated this revision to Diff 507001.Mar 21 2023, 8:54 AM

Address reviewers comments and update tests

skatrak marked 4 inline comments as done.Mar 21 2023, 8:57 AM

Thank you for the feedback! I'll wait for confirmation before landing this patch, in case there are further comments relating to tests.

Thanks for the updates!

I advice against using UPPER case in filenames. Please bear in mind that on Windows and MacOS filenames are case insensitive. It's just less hassle to stick to lower case.


This is confusing - -save-temps should work even when no directory is specified.


Please use LLVM's containers instead (e.g. SmallString or StringRef)


Could you test both variants?


Please use hooks from Path.h for path manipulation.


Why not just print the MLIR module?


IMHO, the fact that this is the 2nd file is not that relevant (what people start printing more files and this becomes the 3rd file?). But I would appreciate a comment explaining that this is the "LLVM IR MLIR file" (i.e. the MLIR file just before lowering to LLVM IR). Similar comment for FIR.mlir.


Could you add a test for this diagnostic? You could try by specifying an invalid output dir.

14 ↗(On Diff #507001)

Why are there no MLIR files here? Same comment for other invocations.

61 ↗(On Diff #507001)

What happens in this case: -save-temps? (no dir is specified)

skatrak updated this revision to Diff 507365.Mar 22 2023, 7:59 AM
skatrak marked 6 inline comments as done.

Changes to tests and path manipulation

Thanks again for helping improving this patch. I made most of the changes requested, and tried to clarify a bit better the approach followed for this implementation, to see if it is acceptable or if some more fundamental changes have to be done.


I'm not sure I completely understand the issue here, by looking at the code you linked to. Maybe it's related to the fact that this function is implemented by creating new files apart from the main output of the current compiler invocation. I developed the idea a bit more in the reply to your comment on "save-temps.f90".

14 ↗(On Diff #507001)

This is because the general way in which -save-temps works is different from what's implemented in this patch for MLIR in flang. In the other cases, the driver splits the work into several frontend invocations, where each step generally produces the input of the next. -save-temps makes sure these intermediate files are kept where the user specified and not deleted.

In this patch, instead of modifying the driver to create a frontend invocation to produce MLIR (generating the *-fir.mlir file), another one to optimize/lower that (generating the *-llvmir.mlir file), and a third one to translate lowered MLIR into LLVM IR, we just forward the flag to the frontend, which creates extra MLIR files at particular spots of the codegen process if the flag is set. Hence, MLIR files don't show in the output of flang-new -###.

61 ↗(On Diff #507001)

The same as if -save-temps=cwd is specified. The test is now updated to check all supported options.

gregrodgers accepted this revision.Mar 23 2023, 7:48 AM

This looks good. Please merge. I found it very useful especially in the context of other generated temp files generated after llvm llinking and optimization in the offload driver. For example just listing the temp files with ls -ltr provides a somewhat visual ordering of the compilation flow.

@awarzynski, can you confirm whether your concerns have been addressed? Thank you!

awarzynski accepted this revision.Mar 23 2023, 12:19 PM

This is a very nice patch, thanks for working on this! A few final nits, but feel free to ignore. LGTM


Apologies, I am blind - you were using print already!

58 ↗(On Diff #507365)

[nit] For MLIR you check the contents of the intermediate files (great!), but then for all other temp files we don't (some of them are binary and that's one reason). Given that testing for MLIR temp files is so different, I'd be tempted to move to a different file. Or at least add a comment here to explain the rationale for testing differently.

14 ↗(On Diff #507001)

So, IIUC, without -emit-llvm-bc there should be no intermediate MLIR files? I would add CHECK-NOT.

skatrak updated this revision to Diff 508099.Mar 24 2023, 8:03 AM

Revision of tests

skatrak marked 3 inline comments as done.Mar 24 2023, 8:15 AM
skatrak added inline comments.

No problem! That makes it two of us, then ;)

58 ↗(On Diff #507365)

Thanks for the suggestion. I separated that into its own test which also contains an explanation of why it's tested differently.

14 ↗(On Diff #507001)

Actually, with this approach there are no changes to the output of flang -### either way. I was using -fc1 -emit-llvm-bc just because that triggers both MLIR temp outputs (the one before any optimizations/lowering, and the one right before LLVM IR generation), but I simplified that a bit by just using flang -c instead and avoid confusion.

awarzynski accepted this revision.Mar 24 2023, 9:08 AM


14 ↗(On Diff #507001)

Ah, I missed that, good point!

This revision was landed with ongoing or failed builds.Mar 24 2023, 10:14 AM
This revision was automatically updated to reflect the committed changes.
skatrak marked an inline comment as done.