[Flang][Driver]Add datalayout before doing LLVM-IR transformation

[Leporacanthicus]

Summary

The earlier available datalyaout allows MLIR to LLVM-IR transformation
to use the datalayout for decisions, such as comparing sizes for
different types of integers.

This should solve https://github.com/llvm/llvm-project/issues/57230

[awarzynski]

Thanks for implementing this @MatsPetersson !

Could add a test to demonstrate the behavior when the input MLIR module does and does not define the data layout?

flang/lib/Frontend/FrontendActions.cpp

571

The comment in FrontendActions.h is a bit confusing:

/// Sets up LLVM's TargetMachine, configures llvmModule accordingly.

With your changes, llvmModule may or may not be configured. And even without your changes, the data layout (extracted from the target machine), is set after the call to setUpTargetMachine (so the LLVM module is not configured accordingly).

I would update this method to focus on "setting up the target machine". And the bit concerned with setting up the triple should probably be extracted.

flang/test/Driver/pic-flags.f90

1

Unrelated change

17

Unrelated change

flang/unittests/Frontend/FrontendActionTest.cpp

181–185

Why is this suddenly needed?

[vzakhari]

flang/lib/Frontend/FrontendActions.cpp

708

I am not sure if we can use string representation of LLVM data layout as MLIR data layout. I would rather follow the Polygeist way and translate LLVM data layout using this method.

754

Do we need this, if MLIR module already has proper data layout? Shouldn't it be translated into LLVM data layout during translation to LLVM IR dialect?

[Leporacanthicus]

flang/lib/Frontend/FrontendActions.cpp

708

Good point. I am trying to test this thoroughly, but I have been busy with other things.

754

Yes. if someone feeds flang-new with a .ll file, it will need to set the DL for LLVM to use later. Because we never created an MLIR module. Hence the check for "is it default" before setting it - we only set it if it wasn't already set.

flang/test/Driver/pic-flags.f90

1

No. We need all the targets that the test runs to to be able to run - now that we set the DL earlier, it fails, where before it didn't.

flang/unittests/Frontend/FrontendActionTest.cpp

181–185

Because when we set the DataLayout earlier, the TargetMachine needs to exist for it to be able to set. It took me a while to understand, but it was clearly the problem, as I discovered once I realized the difference between this test [before these lines were added] and the next one below. We tried to get the TargetMachine, and the LLVM infrastructure has an empty list of target machines, retuns NULL [or end() from some storage]

Previously, we only set DataLayout before turning the LLVM-IR into machine-code, which doesn't happen during this test.

[awarzynski]

Thanks for the clarifications, Mats! I see that with this change you are also making sure that the datalayout is set before the -emit-llvm actions ends. I think that technically that's not required for this change to work - or am I still missing something? In either case, it's not too much noise and we can keep here. But it would be good to point out in the commit message that with this change, flang-new -emit-llvm -S file.f90 will generate an LLVM IR file with correct datalayout present. I hope that I got it right :)

flang/lib/Frontend/FrontendActions.cpp

754

@Leporacanthicus This makes sense, but the name of method that you are using here does not convey that (not your fault!) :) If this was something like this then the intent would be clearer:

if (!llvmModule->getDataLayout())

So, I suggest adding a comment to explain that isDefault() is equivalent to "there is no data layout yet" :)

[Leporacanthicus]

Updates based on review comments:

• Additional MLIR DataLayout.
• Add check lines for datalayout in the MLIR and LLVM output

[awarzynski]

flang/lib/Frontend/FrontendActions.cpp

155–159

Added some suggestion inline (spiting the logic a bit and adding missing comment)

In general, this feels a bit weird. If the lowering bridge provides the MLIR module then it should also configure it correctly, right? i.e. set-up the datalayout. I don't mind this being added here, just thinking about the overall design.

566–567

Why is it not safe to reset it?

I think that it's a good idea to only set it once per compilation. Unless there's a good reasons to allow it multiple times, but I'm not aware of a case requiring that.

755

What happens when consuming an LLVM IR module? In particular, there are 4 scenarios:

• input module contains DL, compiler invocation contains --target=<target>
• input module contains DL, compiler invocation does not contain --target=<target>
• input module does not contain DL, compiler invocation contains --target=<target>
• input module does not contain DL, compiler invocation does not contain --target=<target>

Is the behavior "before" and "after" this change identical? And is it consistent with Clang? I feel that this particular change might have changed the behavior of flang-new.

[Leporacanthicus]

Updates per review comments and local testing.

Mainly, set DataLayout for more conditions, rather than skipping the setting.

flang/lib/Frontend/FrontendActions.cpp

155–159

I looked at this, but it's rather complicated, there isn't enough infrastructure in place to do this.

I plan on raising a ticket for "MLIR support for DataLayout is lacking" (or something like that), but I'd like to have this done first.

566–567

Safe in this case is "doesn't unnecessarily set things that we've already set". I'll reword.

Anyway, turns out we want to set it every time, so no longer there... :)

708

So, it turns out we need both the translation and setting the LLVM datalayout, otherwise we can't "recover" the original layout.

As mentioned above, I intend to raise a ticket for "insufficient DL support in MLIR".

755

Comparing with clang, it behaves the same (except for using SSE instructions in clang, and not in Flang - I don't believe that's to do with my changes tho'). I would like to know what it is, actually, since it probably wouldn't hurt to have in Flang - on the other hand, x86-32 isn't a key target afaik.

Either way, it matches the previous implementation after I tested things.

[vzakhari]

flang/lib/Frontend/FrontendActions.cpp

763

typo: teh

764

typo: differen

[vzakhari]

I accept this (module the typos), but please make sure @awarzynski is okay with the changes. Thank you!

[awarzynski]

flang/lib/Frontend/FrontendActions.cpp

128

Do you need this temporary?

155–159

I looked at this, but it's rather complicated, there isn't enough infrastructure in place to do this.

This is only addressing my 2nd comment in the box above (starting with "In general"). How about the earlier bit? Amongst other things, this comment is incomplete:

// Fetch module from lb, so we can set

I've also made some suggestions inline.

755

Why? :) This comment currently repeats "what" the code does. But it's hard to understand the "why" behind it. Ultimately, we need clear documentation for where the triple comes from.

IIRC, tm is a member of CompilerInvocation. So, tm contains the triple specified on the command line (or the default triple if no triple is specified). And in this block, you are making sure that it's the set-up from CompilerInvocaiton is used to set-up the triple.

This is, IMHO, key. The triple can come from 2 places (3 if you count MLIR modules) and I would like for the code to clearly document this.

flang/test/Driver/pic-flags.f90

2

Unrelated change, please extract to a separate patch.

[Leporacanthicus]

Update comments (clarify/reword, fix typos) and rebase.

[awarzynski]

Thank you @Leporacanthicus , this has been a tricky one! LGTM

flang/lib/Frontend/FrontendActions.cpp

 #include "flang/Parser/unparse.h"
 #include "flang/Semantics/runtime-type-info.h"
 #include "flang/Semantics/semantics.h"
 #include "flang/Semantics/unparse-with-symbols.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/ModuleTranslation.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticFrontend.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"
if (!module || mlir::failed(module->verifyInvariants())) {
       unsigned diagID = ci.getDiagnostics().getCustomDiagID(
           clang::DiagnosticsEngine::Error, "Could not parse FIR");
       ci.getDiagnostics().Report(diagID);
       return false;
     }
 
     mlirModule = std::make_unique<mlir::ModuleOp>(module.release());
     return true;
   }
 
   // Otherwise, generate an MLIR module from the input Fortran source
   if (getCurrentInput().getKind().getLanguage() != Language::Fortran) {
     unsigned diagID = ci.getDiagnostics().getCustomDiagID(
         clang::DiagnosticsEngine::Error,
         "Invalid input type - expecting a Fortran file");
     ci.getDiagnostics().Report(diagID);
     return false;
fir::KindMapping kindMap(mlirCtx.get(), llvm::ArrayRef<fir::KindTy>{
                                               fir::fromDefaultKinds(defKinds)});
   lower::LoweringBridge lb = Fortran::lower::LoweringBridge::create(
       *mlirCtx, ci.getInvocation().getSemanticsContext(), defKinds,
       ci.getInvocation().getSemanticsContext().intrinsics(),
       ci.getInvocation().getSemanticsContext().targetCharacteristics(),
       ci.getParsing().allCooked(), ci.getInvocation().getTargetOpts().triple,
       kindMap, ci.getInvocation().getLoweringOpts());
 
   // Create a parse tree and lower it to FIR
   Fortran::parser::Program &parseTree{*ci.getParsing().parseTree()};
   lb.lower(parseTree, ci.getInvocation().getSemanticsContext());
   mlirModule = std::make_unique<mlir::ModuleOp>(lb.getModule());
 
   // run the default passes.
   mlir::PassManager pm(mlirCtx.get(), mlir::OpPassManager::Nesting::Implicit);
   pm.enableVerifier(/*verifyPasses=*/true);
   pm.addPass(std::make_unique<Fortran::lower::VerifierPass>());
 
   if (mlir::failed(pm.run(*mlirModule))) {
     unsigned diagID = ci.getDiagnostics().getCustomDiagID(
         clang::DiagnosticsEngine::Error,
getCGOptLevel(const Fortran::frontend::CodeGenOptions &opts) {
   case 2:
     return llvm::CodeGenOpt::Default;
   case 3:
     return llvm::CodeGenOpt::Aggressive;
   }
 }
 
 void CodeGenAction::setUpTargetMachine() {
   CompilerInstance &ci = this->getInstance();
 
-  // Set the triple based on the CompilerInvocation set-up
   const std::string &theTriple = ci.getInvocation().getTargetOpts().triple;
+
+  // Set the triple based on the CompilerInvocation set-up
+  if (llvmModule) {
     if (llvmModule->getTargetTriple() != theTriple) {
       ci.getDiagnostics().Report(clang::diag::warn_fe_override_module)
           << theTriple;
+    }
     llvmModule->setTargetTriple(theTriple);
   }
 
   // Create `Target`
   std::string error;
   const llvm::Target *theTarget =
       llvm::TargetRegistry::lookupTarget(theTriple, error);
   assert(theTarget && "Failed to create Target");
void CodeGenAction::runOptimizationPipeline(llvm::raw_pwrite_stream &os) {
 
   if (action == BackendActionTy::Backend_EmitBC)
     mpm.addPass(llvm::BitcodeWriterPass(os));
 
   // Run the passes.
   mpm.run(*llvmModule, mam);
 }
 
+static void setMLIRDataLayout(mlir::ModuleOp &mlirModule,
+                              const llvm::DataLayout &dl) {
+  mlirModule->setAttr(mlir::LLVM::LLVMDialect::getDataLayoutAttrName(),
+                      mlir::StringAttr::get(mlirModule.getContext(),
+                                            dl.getStringRepresentation()));
+}
+
 void CodeGenAction::executeAction() {
   CompilerInstance &ci = this->getInstance();
 
   // If the output stream is a file, generate it and define the corresponding
   // output stream. If a pre-defined output stream is available, we will use
   // that instead.
   //
   // NOTE: `os` is a smart pointer that will be destroyed at the end of this
if (!os) {
       return;
     }
   }
 
   if (action == BackendActionTy::Backend_EmitMLIR) {
     mlirModule->print(ci.isOutputStreamNull() ? *os : ci.getOutputStream());
     return;
   }
+  setUpTargetMachine();
+  if (mlirModule) {
+    const llvm::DataLayout &dl = tm->createDataLayout();
+    setMLIRDataLayout(*mlirModule, dl);
+  }
 
   // Generate an LLVM module if it's not already present (it will already be
   // present if the input file is an LLVM IR/BC file).
   if (!llvmModule)
     generateLLVMIR();
 
+  if (llvmModule->getDataLayout().isDefault()) {
+    llvmModule->setDataLayout(tm->createDataLayout());
+  }
   // Run LLVM's middle-end (i.e. the optimizer).
   runOptimizationPipeline(*os);
 
   if (action == BackendActionTy::Backend_EmitLL) {
     llvmModule->print(ci.isOutputStreamNull() ? *os : ci.getOutputStream(),
                       /*AssemblyAnnotationWriter=*/nullptr);
     return;
   }
 
-  setUpTargetMachine();
-  llvmModule->setDataLayout(tm->createDataLayout());
-
   if (action == BackendActionTy::Backend_EmitBC) {
     // This action has effectively been completed in runOptimizationPipeline.
     return;
   }
 
   // Run LLVM's backend and generate either assembly or machine code
   if (action == BackendActionTy::Backend_EmitAssembly ||
       action == BackendActionTy::Backend_EmitObj) {

flang/test/Driver/emit-llvm.f90

 ! Test the `-emit-llvm` option
 
 ! UNSUPPORTED: system-windows
 ! Windows is currently not supported in flang/lib/Optimizer/CodeGen/Target.cpp
 
 ! RUN: %flang_fc1 -emit-llvm %s -o - | FileCheck %s
 
 ! CHECK: ; ModuleID = 'FIRModule'
+! CHECK: target datalayout =
 ! CHECK: define void @_QQmain()
 ! CHECK-NEXT:  ret void
 ! CHECK-NEXT: }
 
 end program

flang/test/Driver/pic-flags.f90

+! REQUIRES: aarch64-registered-target && x86-registered-target && arm-registered-target
 ! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fno-pie 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-STATIC,CHECK-STATIC-IR
 
 ! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-PIE-LEVEL2,CHECK-PIE-LEVEL2-IR
 ! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fpie 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-PIE-LEVEL1,CHECK-PIE-LEVEL1-IR
 ! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fPIE 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-PIE-LEVEL2,CHECK-PIE-LEVEL2-IR
 
 ! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fpic 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-PIC-LEVEL1,CHECK-PIC-LEVEL1-IR
 ! RUN: %flang -v -S -emit-llvm -o - %s --target=aarch64-linux-gnu -fPIC 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-PIC-LEVEL2,CHECK-PIC-LEVEL2-IR
 
 ! RUN: %flang -v -### -o - %s --target=i386-apple-darwin -mdynamic-no-pic 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-DYNAMIC-NO-PIC-32
 ! RUN: %flang -v -### -o - %s --target=x86_64-apple-darwin -mdynamic-no-pic 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-DYNAMIC-NO-PIC-64
 
 ! RUN: %flang -v -### -o - %s --target=arm-none-eabi -fropi 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-ROPI
 ! RUN: %flang -v -### -o - %s --target=arm-none-eabi -frwpi 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-RWPI
 ! RUN: %flang -v -### -o - %s --target=arm-none-eabi -fropi -frwpi 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-ROPI-RWPI
 
-
 ! CHECK: -fc1
 
 
 !! -fno-pie.
 ! CHECK-STATIC: -mrelocation-model static
 ! CHECK-STATIC-NOT: -pic
 
 ! CHECK-STATIC-IR-NOT: {{PIE|PIC}} Level

flang/unittests/Frontend/FrontendActionTest.cpp

TEST_F(FrontendActionTest, EmitLLVM) {
 
   // Set-up the action kind.
   compInst.getInvocation().getFrontendOpts().programAction = EmitLLVM;
 
   // Set-up default target triple.
   compInst.getInvocation().getTargetOpts().triple =
       llvm::Triple::normalize(llvm::sys::getDefaultTargetTriple());
 
+  // Initialise LLVM backend
+  llvm::InitializeAllTargets();
+  llvm::InitializeAllTargetMCs();
+  llvm::InitializeAllAsmPrinters();
+
   // Set-up the output stream. We are using output buffer wrapped as an output
   // stream, as opposed to an actual file (or a file descriptor).
   llvm::SmallVector<char> outputFileBuffer;
   std::unique_ptr<llvm::raw_pwrite_stream> outputFileStream(
       new llvm::raw_svector_ostream(outputFileBuffer));
   compInst.setOutputStream(std::move(outputFileStream));
 
   // Execute the action.