[PATCH] D146075: [flang][driver][openmp] Write MLIR for -save-temps

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski accepted this revision.
awarzynski added a comment.

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



================
Comment at: flang/lib/Frontend/FrontendActions.cpp:103
+  std::error_code ec;
+  llvm::ToolOutputFile out(outDir + outFile, ec, llvm::sys::fs::OF_Text);
+  if (ec)
----------------
skatrak wrote:
> awarzynski wrote:
> > Why not just [[ https://github.com/llvm/llvm-project/blob/3b1951aceb8c58acd3d5c5ba2d042ad3d03c13c4/flang/lib/Frontend/FrontendActions.cpp#L793 | print ]] the MLIR module?
> 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".
Apologies, I am blind - you were using `print` already!


================
Comment at: flang/test/Driver/save-temps.f90:58
+!--------------------------
+! MLIR generation
+!--------------------------
----------------
[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.


================
Comment at: flang/test/Driver/save-temps.f90:14
 ! CHECK-NEXT: "-o" "save-temps.o"
 ! CHECK-NEXT: "-o" "a.out"
 
----------------
skatrak wrote:
> awarzynski wrote:
> > Why are there no MLIR files here? Same comment for other invocations.
> 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 -###`.
So, IIUC, without `-emit-llvm-bc` there should be no intermediate MLIR files? I would add `CHECK-NOT`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146075/new/

https://reviews.llvm.org/D146075