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

[Sergio Afonso via Phabricator via cfe-commits]

skatrak marked 3 inline comments as done.
skatrak added inline comments.


================
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)
----------------
awarzynski wrote:
> 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!
No problem! That makes it two of us, then ;)


================
Comment at: flang/test/Driver/save-temps.f90:58
+!--------------------------
+! MLIR generation
+!--------------------------
----------------
awarzynski wrote:
> [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.
Thanks for the suggestion. I separated that into its own test which also contains an explanation of why it's tested differently.


================
Comment at: flang/test/Driver/save-temps.f90:14
 ! CHECK-NEXT: "-o" "save-temps.o"
 ! CHECK-NEXT: "-o" "a.out"
 
----------------
awarzynski wrote:
> 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`.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146075