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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
