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

[skatrak]

Summary

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.

[Leporacanthicus]

LGTM.

[awarzynski]

Please add tests ;)

[tschuett]

flang/lib/Frontend/FrontendActions.cpp

78

static is cuter.

[tschuett]

What is the policy on trivial braces in the frontend?

[awarzynski]

In D146075#4196998, @tschuett wrote:

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 https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements.

[tschuett]

Frontend is in the file name.

flang/lib/Frontend/FrontendActions.cpp

80

useless braces

92

useless braces

107

useless braces

[skatrak]

Address reviewers comments and update tests

[skatrak]

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

[awarzynski]

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.

flang/lib/Frontend/FrontendActions.cpp

80

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

83–84

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

87–88

Could you test both variants?

97–101

Please use hooks from Path.h for path manipulation.

104

Why not just print the MLIR module?

619

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.

623

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

flang/test/Driver/save-temps.f90

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]

Changes to tests and path manipulation

[skatrak]

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.

flang/lib/Frontend/FrontendActions.cpp

104

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".

flang/test/Driver/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]

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.

[skatrak]

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

[awarzynski]

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

flang/lib/Frontend/FrontendActions.cpp

104

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

flang/test/Driver/save-temps.f90

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]

Revision of tests

[skatrak]

flang/lib/Frontend/FrontendActions.cpp

104

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

flang/test/Driver/save-temps.f90

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]

LGTM!

flang/test/Driver/save-temps.f90

14 ↗

(On Diff #507001)

Ah, I missed that, good point!

clang/include/clang/Driver/Options.td

def fno_rtlib_add_rpath: Flag<["-"], "fno-rtlib-add-rpath">, Flags<[NoArgumentUnused]>,
   HelpText<"Do not add -rpath with architecture-specific resource directory to the linker flags. "
   "When --hip-link is specified, do not add -rpath with HIP runtime library directory to the linker flags">;
 def offload_add_rpath: Flag<["--"], "offload-add-rpath">, Flags<[NoArgumentUnused]>,
   Alias<frtlib_add_rpath>;
 def no_offload_add_rpath: Flag<["--"], "no-offload-add-rpath">, Flags<[NoArgumentUnused]>,
   Alias<frtlib_add_rpath>;
 def r : Flag<["-"], "r">, Flags<[LinkerInput,NoArgumentUnused]>,
         Group<Link_Group>;
-def save_temps_EQ : Joined<["-", "--"], "save-temps=">, Flags<[CC1Option, FlangOption, NoXarchOption]>,
+def save_temps_EQ : Joined<["-", "--"], "save-temps=">, Flags<[CC1Option, FlangOption, FC1Option, NoXarchOption]>,
   HelpText<"Save intermediate compilation results.">;
-def save_temps : Flag<["-", "--"], "save-temps">, Flags<[FlangOption, NoXarchOption]>,
+def save_temps : Flag<["-", "--"], "save-temps">, Flags<[FlangOption, FC1Option, NoXarchOption]>,
   Alias<save_temps_EQ>, AliasArgs<["cwd"]>,
   HelpText<"Save intermediate compilation results">;
 def save_stats_EQ : Joined<["-", "--"], "save-stats=">, Flags<[NoXarchOption]>,
   HelpText<"Save llvm statistics.">;
 def save_stats : Flag<["-", "--"], "save-stats">, Flags<[NoXarchOption]>,
   Alias<save_stats_EQ>, AliasArgs<["cwd"]>,
   HelpText<"Save llvm statistics.">;
 def via_file_asm : Flag<["-", "--"], "via-file-asm">, InternalDebugOpt,

clang/lib/Driver/ToolChains/Flang.cpp

if (Output.isFilename()) {
     CmdArgs.push_back("-o");
     CmdArgs.push_back(Output.getFilename());
   } else {
     assert(Output.isNothing() && "Invalid output.");
   }
 
   assert(Input.isFilename() && "Invalid input.");
 
+  if (Args.getLastArg(options::OPT_save_temps_EQ))
+    Args.AddLastArg(CmdArgs, options::OPT_save_temps_EQ);
+
   addDashXForInput(Args, Input, CmdArgs);
 
   CmdArgs.push_back(Input.getFilename());
 
   // TODO: Replace flang-new with flang once the new driver replaces the
   // throwaway driver
   const char *Exec = Args.MakeArgString(D.GetProgramPath("flang-new", TC));
   C.addCommand(std::make_unique<Command>(JA, *this,
                                          ResponseFileSupport::AtFileUTF8(),
                                          Exec, CmdArgs, Inputs, Output));
 }
 
 Flang::Flang(const ToolChain &TC) : Tool("flang-new", "flang frontend", TC) {}
 
 Flang::~Flang() {}

flang/include/flang/Frontend/CodeGenOptions.h

 #define LLVM_CLANG_BASIC_CODEGENOPTIONS_H
 
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Instrumentation/AddressSanitizerOptions.h"
 #include <map>
 #include <memory>
+#include <optional>
 #include <string>
 #include <vector>
 
 namespace Fortran::frontend {
 
 /// Bitfields of CodeGenOptions, split out from CodeGenOptions to ensure
 /// that this large collection of bitfields is a trivial class type.
 class CodeGenOptionsBase {

 public:
   /// The paths to the pass plugins that were registered using -fpass-plugin.
   std::vector<std::string> LLVMPassPlugins;
 
   /// List of filenames passed in using the -fembed-offload-object option. These
   /// are offloading binaries containing device images and metadata.
   std::vector<std::string> OffloadObjects;
 
+  /// The directory where temp files are stored if specified by -save-temps
+  std::optional<std::string> SaveTempsDir;
+
   // Define accessors/mutators for code generation options of enumeration type.
 #define CODEGENOPT(Name, Bits, Default)
 #define ENUM_CODEGENOPT(Name, Type, Bits, Default)                             \
   Type get##Name() const { return static_cast<Type>(Name); }                   \
   void set##Name(Type Value) { Name = static_cast<unsigned>(Value); }
 #include "flang/Frontend/CodeGenOptions.def"
 
   CodeGenOptions();
 };
 
 } // end namespace Fortran::frontend
 
 #endif

flang/lib/Frontend/CompilerInvocation.cpp

if (const llvm::opt::Arg *a =
     llvm::StringRef s = a->getValue();
     assert((s == "full" || s == "thin") && "Unknown LTO mode.");
     if (s == "full")
       opts.PrepareForFullLTO = true;
     else
       opts.PrepareForThinLTO = true;
   }
 
+  if (auto *a = args.getLastArg(clang::driver::options::OPT_save_temps_EQ))
+    opts.SaveTempsDir = a->getValue();
+
   // -mrelocation-model option.
   if (const llvm::opt::Arg *a =
           args.getLastArg(clang::driver::options::OPT_mrelocation_model)) {
     llvm::StringRef modelName = a->getValue();
     auto relocModel =
         llvm::StringSwitch<std::optional<llvm::Reloc::Model>>(modelName)
             .Case("static", llvm::Reloc::Static)
             .Case("pic", llvm::Reloc::PIC_)

flang/lib/Frontend/FrontendActions.cpp

 //===--- FrontendActions.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Coding style: https://mlir.llvm.org/getting_started/DeveloperGuide/
 //
 //===----------------------------------------------------------------------===//
 
 #include "flang/Frontend/FrontendActions.h"
 #include "flang/Common/default-kinds.h"
 #include "flang/Frontend/CompilerInstance.h"
+#include "flang/Frontend/CompilerInvocation.h"
 #include "flang/Frontend/FrontendOptions.h"
 #include "flang/Frontend/PreprocessorOptions.h"
 #include "flang/Lower/Bridge.h"
 #include "flang/Lower/PFTBuilder.h"
 #include "flang/Lower/Support/Verifier.h"
 #include "flang/Optimizer/Dialect/Support/FIRContext.h"
 #include "flang/Optimizer/Dialect/Support/KindMapping.h"
 #include "flang/Optimizer/Support/InitFIR.h"

 #include "mlir/IR/Dialect.h"
 #include "mlir/Parser/Parser.h"
 #include "mlir/Pass/PassManager.h"
 #include "mlir/Support/LLVM.h"
 #include "mlir/Target/LLVMIR/Import.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Bitcode/BitcodeWriterPass.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Object/OffloadBinary.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Passes/PassPlugin.h"
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 #include <memory>
+#include <system_error>
 
 using namespace Fortran::frontend;
 
 // Declare plugin extension function declarations.
 #define HANDLE_EXTENSION(Ext)                                                  \
   llvm::PassPluginLibraryInfo get##Ext##PluginInfo();
 #include "llvm/Support/Extension.def"
 
+/// Save the given \c mlirModule to a temporary .mlir file, in a location
+/// decided by the -save-temps flag. No files are produced if the flag is not
+/// specified.
+static bool saveMLIRTempFile(const CompilerInvocation &ci,
+                             mlir::ModuleOp mlirModule,
+                             llvm::StringRef inputFile,
+                             llvm::StringRef outputTag) {
+  if (!ci.getCodeGenOpts().SaveTempsDir.has_value())
+    return true;
+
+  const llvm::StringRef compilerOutFile = ci.getFrontendOpts().outputFile;
+  const llvm::StringRef saveTempsDir = ci.getCodeGenOpts().SaveTempsDir.value();
+  auto dir = llvm::StringSwitch<llvm::StringRef>(saveTempsDir)
+                 .Case("cwd", "")
+                 .Case("obj", llvm::sys::path::parent_path(compilerOutFile))
+                 .Default(saveTempsDir);
+
+  // Build path from the compiler output file name, triple, cpu and OpenMP
+  // information
+  llvm::SmallString<256> path(dir);
+  llvm::sys::path::append(path, llvm::sys::path::stem(inputFile) + "-" +
+                                    outputTag + ".mlir");
+
+  std::error_code ec;
+  llvm::ToolOutputFile out(path, ec, llvm::sys::fs::OF_Text);
+  if (ec)
+    return false;
+
+  mlirModule->print(out.os());
+  out.os().close();
+  out.keep();
+
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 // Custom BeginSourceFileAction
 //===----------------------------------------------------------------------===//
 
 bool PrescanAction::beginSourceFileAction() { return runPrescan(); }
 
 bool PrescanAndParseAction::beginSourceFileAction() {
   return runPrescan() && runParse();
bool CodeGenAction::beginSourceFileAction() {
   if (mlir::failed(pm.run(*mlirModule))) {
     unsigned diagID = ci.getDiagnostics().getCustomDiagID(
         clang::DiagnosticsEngine::Error,
         "verification of lowering to FIR failed");
     ci.getDiagnostics().Report(diagID);
     return false;
   }
 
+  // Print initial full MLIR module, before lowering or transformations, if
+  // -save-temps has been specified.
+  if (!saveMLIRTempFile(ci.getInvocation(), *mlirModule, getCurrentFile(),
+                        "fir")) {
+    unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+        clang::DiagnosticsEngine::Error, "Saving MLIR temp file failed");
+    ci.getDiagnostics().Report(diagID);
+    return false;
+  }
+
   return true;
 }
 
 //===----------------------------------------------------------------------===//
 // Custom ExecuteAction
 //===----------------------------------------------------------------------===//
 void InputOutputTestAction::executeAction() {
   CompilerInstance &ci = getInstance();
void CodeGenAction::generateLLVMIR() {
 
   // run the pass manager
   if (!mlir::succeeded(pm.run(*mlirModule))) {
     unsigned diagID = ci.getDiagnostics().getCustomDiagID(
         clang::DiagnosticsEngine::Error, "Lowering to LLVM IR failed");
     ci.getDiagnostics().Report(diagID);
   }
 
+  // Print final MLIR module, just before translation into LLVM IR, if
+  // -save-temps has been specified.
+  if (!saveMLIRTempFile(ci.getInvocation(), *mlirModule, getCurrentFile(),
+                        "llvmir")) {
+    unsigned diagID = ci.getDiagnostics().getCustomDiagID(
+        clang::DiagnosticsEngine::Error, "Saving MLIR temp file failed");
+    ci.getDiagnostics().Report(diagID);
+    return;
+  }
+
   // Translate to LLVM IR
   std::optional<llvm::StringRef> moduleName = mlirModule->getName();
   llvmModule = mlir::translateModuleToLLVMIR(
       *mlirModule, *llvmCtx, moduleName ? *moduleName : "FIRModule");
 
   if (!llvmModule) {
     unsigned diagID = ci.getDiagnostics().getCustomDiagID(
         clang::DiagnosticsEngine::Error, "failed to create the LLVM module");

flang/test/Driver/driver-help.f90

 ! HELP-FC1-NEXT:                        The relocation model to use
 ! HELP-FC1-NEXT: -nocpp                 Disable predefined and command line preprocessor macros
 ! HELP-FC1-NEXT: -o <file>              Write output to <file>
 ! HELP-FC1-NEXT: -pedantic              Warn on language extensions
 ! HELP-FC1-NEXT: -pic-is-pie             File is for a position independent executable
 ! HELP-FC1-NEXT: -pic-level <value>      Value for __PIC__
 ! HELP-FC1-NEXT: -plugin <name>         Use the named plugin action instead of the default action (use "help" to list available options)
 ! HELP-FC1-NEXT: -P                     Disable linemarker output in -E mode
+! HELP-FC1-NEXT: -save-temps=<value>    Save intermediate compilation results.
+! HELP-FC1-NEXT: -save-temps            Save intermediate compilation results
 ! HELP-FC1-NEXT: -std=<value>           Language standard to compile for
 ! HELP-FC1-NEXT: -S                     Only run preprocess and compilation steps
 ! HELP-FC1-NEXT: -target-cpu <value>    Target a specific cpu type
 ! HELP-FC1-NEXT: -target-feature <value> Target specific attributes
 ! HELP-FC1-NEXT: -test-io               Run the InputOuputTest action. Use for development and testing only.
 ! HELP-FC1-NEXT: -triple <value>        Specify target triple (e.g. i686-apple-darwin9)
 ! HELP-FC1-NEXT: -U <macro>             Undefine macro <macro>
 ! HELP-FC1-NEXT: -version               Print the compiler version
 ! HELP-FC1-NEXT: -W<warning>            Enable the specified warning
 
 ! ERROR: error: unknown argument '-helps'; did you mean '-help'

flang/test/Driver/frontend-forwarding.f90

 ! RUN:     -fconvert=little-endian \
 ! RUN:     -ffp-contract=fast \
 ! RUN:     -fno-honor-nans \
 ! RUN:     -fapprox-func \
 ! RUN:     -fno-signed-zeros \
 ! RUN:     -fassociative-math \
 ! RUN:     -freciprocal-math \
 ! RUN:     -fpass-plugin=Bye%pluginext \
-! RUN:     -mllvm -print-before-all\
+! RUN:     -mllvm -print-before-all \
+! RUN:     -save-temps=obj \
 ! RUN:     -P \
 ! RUN:   | FileCheck %s
 
 ! CHECK: "-P"
 ! CHECK: "-finput-charset=utf-8"
 ! CHECK: "-fdefault-double-8"
 ! CHECK: "-fdefault-integer-8"
 ! CHECK: "-fdefault-real-8"
 ! CHECK: "-flarge-sizes"
 ! CHECK: "-ffp-contract=fast"
 ! CHECK: "-menable-no-nans"
 ! CHECK: "-fapprox-func"
 ! CHECK: "-fno-signed-zeros"
 ! CHECK: "-mreassociate"
 ! CHECK: "-freciprocal-math"
 ! CHECK: "-fconvert=little-endian"
 ! CHECK: "-fpass-plugin=Bye
 ! CHECK: "-mllvm" "-print-before-all"
+! CHECK: "-save-temps=obj"

flang/test/Driver/save-mlir-temps.f90

+! Tests for the `-save-temps` flag. Instead of checking the commands generated
+! by the driver with `-###` (like the save-temps.f90 test does), here we check
+! that the MLIR files are actually produced in the specified location because
+! the driver does not generate specific passes for MLIR. Instead, they are
+! generated during code generation as additional outputs.
+
+! As `flang` does not implement `-fc1as` (i.e. a driver for the integrated
+! assembler), we need to use `-fno-integrated-as` here.
+
+! UNSUPPORTED: system-windows
+
+!--------------------------
+! Invalid output directory
+!--------------------------
+! RUN: not %flang_fc1 -emit-llvm-bc -save-temps=#invalid-dir -o - %s 2>&1 | FileCheck %s -check-prefix=MLIR-ERROR
+! MLIR-ERROR: error: Saving MLIR temp file failed
+
+!--------------------------
+! Save to cwd
+!--------------------------
+! RUN: rm -rf %t && mkdir -p %t
+! RUN: pushd %t && %flang -c -fno-integrated-as -save-temps=cwd -o out.o %s 2>&1
+! RUN: FileCheck %s -input-file=save-mlir-temps-fir.mlir -check-prefix=MLIR-FIR
+! RUN: FileCheck %s -input-file=save-mlir-temps-llvmir.mlir -check-prefix=MLIR-LLVMIR
+! RUN: popd
+
+! RUN: rm -rf %t && mkdir -p %t
+! RUN: pushd %t && %flang -c -fno-integrated-as -save-temps -o out.o %s 2>&1
+! RUN: FileCheck %s -input-file=save-mlir-temps-fir.mlir -check-prefix=MLIR-FIR
+! RUN: FileCheck %s -input-file=save-mlir-temps-llvmir.mlir -check-prefix=MLIR-LLVMIR
+! RUN: popd
+
+!--------------------------
+! Save to output directory
+!--------------------------
+! RUN: rm -rf %t && mkdir -p %t
+! RUN: %flang -c -fno-integrated-as -save-temps=obj -o %t/out.o %s 2>&1
+! RUN: FileCheck %s -input-file=%t/save-mlir-temps-fir.mlir -check-prefix=MLIR-FIR
+! RUN: FileCheck %s -input-file=%t/save-mlir-temps-llvmir.mlir -check-prefix=MLIR-LLVMIR
+
+!--------------------------
+! Save to specific directory
+!--------------------------
+! RUN: rm -rf %t && mkdir -p %t
+! RUN: %flang -c -fno-integrated-as -save-temps=%t -o %t/out.o %s 2>&1
+! RUN: FileCheck %s -input-file=%t/save-mlir-temps-fir.mlir -check-prefix=MLIR-FIR
+! RUN: FileCheck %s -input-file=%t/save-mlir-temps-llvmir.mlir -check-prefix=MLIR-LLVMIR
+
+!--------------------------
+! Content to check from the MLIR outputs
+!--------------------------
+! MLIR-FIR-NOT: llvm.func
+! MLIR-FIR: func.func @{{.*}}main() {
+
+! MLIR-FIR-NOT: func.func
+! MLIR-LLVMIR: llvm.func @{{.*}}main() {
+
+end program