awarzynski added a comment.
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.
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:79
+ llvm::StringRef outputTag) {
+ if (!ci.getCodeGenOpts().SaveTempsDir.has_value())
+ return true;
----------------
This is confusing - `-save-temps` should work even when no directory is specified.
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:82-83
+
+ const std::string &compilerOutFile = ci.getFrontendOpts().outputFile;
+ const std::string &dir = ci.getCodeGenOpts().SaveTempsDir.value();
+ std::string outDir =
----------------
Please use LLVM's containers instead (e.g. `SmallString` or `StringRef`)
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:86-87
+ llvm::StringSwitch<std::string>(dir)
+ .Case("cwd", "")
+ .Case("obj", llvm::sys::path::parent_path(compilerOutFile).str())
+ .Default(dir);
----------------
Could you test both variants?
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:96-100
+ std::string outFile = input.substr(0, input.find_last_of('.'))
+ .str()
+ .append("-")
+ .append(outputTag.str())
+ .append(".mlir");
----------------
Please use hooks from [[ https://github.com/llvm/llvm-project/blob/3b1951aceb8c58acd3d5c5ba2d042ad3d03c13c4/llvm/include/llvm/Support/Path.h#L213-L229 | Path.h ]] for path manipulation.
================
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)
----------------
Why not just [[ https://github.com/llvm/llvm-project/blob/3b1951aceb8c58acd3d5c5ba2d042ad3d03c13c4/flang/lib/Frontend/FrontendActions.cpp#L793 | print ]] the MLIR module?
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:624
+ // LLVMIR.mlir: This is 2nd -save-temps file created for mlir
+ if (!saveMLIRTempFile(ci.getInvocation(), *mlirModule, getCurrentFile(),
----------------
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.
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:628
+ unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+ clang::DiagnosticsEngine::Error, "Saving MLIR temp file failed");
+ ci.getDiagnostics().Report(diagID);
----------------
Could you add a test for this diagnostic? You could try by specifying an invalid output dir.
================
Comment at: flang/test/Driver/save-temps.f90:14
! CHECK-NEXT: "-o" "save-temps.o"
! CHECK-NEXT: "-o" "a.out"
----------------
Why are there no MLIR files here? Same comment for other invocations.
================
Comment at: flang/test/Driver/save-temps.f90:61
+! RUN: rm -rf %t && mkdir -p %t
+! RUN: %flang_fc1 -emit-llvm-bc -save-temps=obj -o %t/out.bc %s 2>&1
+! RUN: FileCheck %s -input-file=%t/save-temps-FIR.mlir -check-prefix=MLIR-FIR
----------------
What happens in this case: `-save-temps`? (no dir is specified)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146075/new/
https://reviews.llvm.org/D146075