[flang] Add -mlink-builtin-bitcode option to fc1 #94763

jsjodin · 2024-06-07T15:37:11Z

This patch enables the -mlink-builtin-bitcode flag in fc1 so that bitcode libraries can be linked in. This is needed for OpenMP offloading libraries.

llvmbot · 2024-06-07T15:37:42Z

@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang

Author: Jan Leyonberg (jsjodin)

Changes

This patch enables the -mlink-builtin-bitcode flag in fc1 so that bitcode libraries can be linked in. This is needed for OpenMP offloading libraries.

Full diff: https://github.com/llvm/llvm-project/pull/94763.diff

7 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+6-3)
(modified) flang/include/flang/Frontend/CodeGenOptions.h (+4)
(modified) flang/include/flang/Frontend/FrontendActions.h (+4-1)
(modified) flang/lib/Frontend/CompilerInvocation.cpp (+5)
(modified) flang/lib/Frontend/FrontendActions.cpp (+56)
(added) flang/test/Driver/Inputs/bclib.bc ()
(added) flang/test/Driver/mlink-builtin-bc.f90 (+18)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index d44faa55c456f..490538ce753e0 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -7028,6 +7028,12 @@ def as_secure_log_file : Separate<["-"], "as-secure-log-file">,
 
 } // let Visibility = [CC1Option, CC1AsOption]
 
+let Visibility = [CC1Option, FC1Option] in {
+def mlink_builtin_bitcode : Separate<["-"], "mlink-builtin-bitcode">,
+  HelpText<"Link and internalize needed symbols from the given bitcode file "
+           "before performing optimizations.">;
+} // let Visibility = [CC1Option, FC1Option]
+
 let Visibility = [CC1Option] in {
 
 def llvm_verify_each : Flag<["-"], "llvm-verify-each">,
@@ -7130,9 +7136,6 @@ defm constructor_aliases : BoolMOption<"constructor-aliases",
           " emitting complete constructors and destructors as aliases when possible">>;
 def mlink_bitcode_file : Separate<["-"], "mlink-bitcode-file">,
   HelpText<"Link the given bitcode file before performing optimizations.">;
-def mlink_builtin_bitcode : Separate<["-"], "mlink-builtin-bitcode">,
-  HelpText<"Link and internalize needed symbols from the given bitcode file "
-           "before performing optimizations.">;
 defm link_builtin_bitcode_postopt: BoolMOption<"link-builtin-bitcode-postopt",
   CodeGenOpts<"LinkBitcodePostopt">, DefaultFalse,
   PosFlag<SetTrue, [], [ClangOption], "Link builtin bitcodes after the "
diff --git a/flang/include/flang/Frontend/CodeGenOptions.h b/flang/include/flang/Frontend/CodeGenOptions.h
index 918192abae724..3bc5d93c4c43e 100644
--- a/flang/include/flang/Frontend/CodeGenOptions.h
+++ b/flang/include/flang/Frontend/CodeGenOptions.h
@@ -56,6 +56,10 @@ class CodeGenOptions : public CodeGenOptionsBase {
   /// are offloading binaries containing device images and metadata.
   std::vector<std::string> OffloadObjects;
 
+  /// List of filenames passed in using the -mlink-builtin-bitcode. These
+  /// are bc libraries that should be linked in and internalized;
+  std::vector<std::string> BuiltinBCLibs;
+
   /// The directory where temp files are stored if specified by -save-temps
   std::optional<std::string> SaveTempsDir;
 
diff --git a/flang/include/flang/Frontend/FrontendActions.h b/flang/include/flang/Frontend/FrontendActions.h
index 7823565eb815f..0a9a9c3401d1d 100644
--- a/flang/include/flang/Frontend/FrontendActions.h
+++ b/flang/include/flang/Frontend/FrontendActions.h
@@ -223,9 +223,12 @@ class CodeGenAction : public FrontendAction {
   std::unique_ptr<llvm::LLVMContext> llvmCtx;
   std::unique_ptr<llvm::Module> llvmModule;
 
-  /// Embeds offload objects given with specified with -fembed-offload-object
+  /// Embeds offload objects specified with -fembed-offload-object
   void embedOffloadObjects();
 
+  /// Links in BC libraries spefified with -fmlink-builtin-bitcode
+  void linkBuiltinBCLibs();
+
   /// Runs pass pipeline to lower HLFIR into FIR
   void lowerHLFIRToFIR();
 
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index f64a939b785ef..f96d72f1ad691 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -347,6 +347,11 @@ static void parseCodeGenArgs(Fortran::frontend::CodeGenOptions &opts,
   if (auto *a = args.getLastArg(clang::driver::options::OPT_save_temps_EQ))
     opts.SaveTempsDir = a->getValue();
 
+  // -mlink-builtin-bitcode
+  for (auto *a :
+       args.filtered(clang::driver::options::OPT_mlink_builtin_bitcode))
+    opts.BuiltinBCLibs.push_back(a->getValue());
+
   // -mrelocation-model option.
   if (const llvm::opt::Arg *a =
           args.getLastArg(clang::driver::options::OPT_mrelocation_model)) {
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index b1b6391f1439c..c2e6af58ffd79 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -43,6 +43,8 @@
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/FileSystemOptions.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
@@ -54,6 +56,7 @@
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/IRReader/IRReader.h"
+#include "llvm/Linker/Linker.h"
 #include "llvm/Object/OffloadBinary.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Passes/PassPlugin.h"
@@ -68,6 +71,7 @@
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/TargetParser/RISCVISAInfo.h"
 #include "llvm/TargetParser/RISCVTargetParser.h"
+#include "llvm/Transforms/IPO/Internalize.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 #include <memory>
 #include <system_error>
@@ -1146,6 +1150,54 @@ void CodeGenAction::embedOffloadObjects() {
   }
 }
 
+void CodeGenAction::linkBuiltinBCLibs() {
+  auto options = clang::FileSystemOptions();
+  clang::FileManager fileManager(options);
+  CompilerInstance &ci = this->getInstance();
+  const auto &cgOpts = ci.getInvocation().getCodeGenOpts();
+
+  std::vector<std::unique_ptr<llvm::Module>> modules;
+
+  // Load LLVM modules
+  for (llvm::StringRef bcLib : cgOpts.BuiltinBCLibs) {
+    auto BCBuf = fileManager.getBufferForFile(bcLib);
+    if (!BCBuf) {
+      auto diagID = ci.getDiagnostics().getCustomDiagID(
+          clang::DiagnosticsEngine::Error, "could not open '%0' for linking");
+      ci.getDiagnostics().Report(diagID) << bcLib;
+      return;
+    }
+
+    llvm::Expected<std::unique_ptr<llvm::Module>> ModuleOrErr =
+        getOwningLazyBitcodeModule(std::move(*BCBuf), *llvmCtx);
+    if (!ModuleOrErr) {
+      auto diagID = ci.getDiagnostics().getCustomDiagID(
+          clang::DiagnosticsEngine::Error, "error loading '%0' for linking");
+      ci.getDiagnostics().Report(diagID) << bcLib;
+      return;
+    }
+    modules.push_back(std::move(ModuleOrErr.get()));
+  }
+
+  // Link modules and internalize functions
+  for (auto &module : modules) {
+    bool Err;
+    Err = llvm::Linker::linkModules(
+        *llvmModule, std::move(module), llvm::Linker::Flags::LinkOnlyNeeded,
+        [](llvm::Module &M, const llvm::StringSet<> &GVS) {
+          llvm::internalizeModule(M, [&GVS](const llvm::GlobalValue &GV) {
+            return !GV.hasName() || (GVS.count(GV.getName()) == 0);
+          });
+        });
+    if (Err) {
+      auto diagID = ci.getDiagnostics().getCustomDiagID(
+          clang::DiagnosticsEngine::Error, "link error when linking '%0'");
+      ci.getDiagnostics().Report(diagID) << module->getSourceFileName();
+      return;
+    }
+  }
+}
+
 static void reportOptRecordError(llvm::Error e, clang::DiagnosticsEngine &diags,
                                  const CodeGenOptions &codeGenOpts) {
   handleAllErrors(
@@ -1237,6 +1289,10 @@ void CodeGenAction::executeAction() {
   llvmModule->setTargetTriple(theTriple);
   llvmModule->setDataLayout(targetMachine.createDataLayout());
 
+  // Link in builtin bitcode libraries
+  if (!codeGenOpts.BuiltinBCLibs.empty())
+    linkBuiltinBCLibs();
+
   // Embed offload objects specified with -fembed-offload-object
   if (!codeGenOpts.OffloadObjects.empty())
     embedOffloadObjects();
diff --git a/flang/test/Driver/Inputs/bclib.bc b/flang/test/Driver/Inputs/bclib.bc
new file mode 100644
index 0000000000000..bf97a6cd3dbe1
Binary files /dev/null and b/flang/test/Driver/Inputs/bclib.bc differ
diff --git a/flang/test/Driver/mlink-builtin-bc.f90 b/flang/test/Driver/mlink-builtin-bc.f90
new file mode 100644
index 0000000000000..0d655a24a7a63
--- /dev/null
+++ b/flang/test/Driver/mlink-builtin-bc.f90
@@ -0,0 +1,18 @@
+
+!----------
+! RUN lines
+!----------
+! Embed something that can be easily checked
+! RUN: %flang_fc1 -emit-llvm -triple x86_64-unknown-linux-gnu -o - -mlink-builtin-bitcode %S/Inputs/bclib.bc %s 2>&1 | FileCheck %s
+
+! CHECK: define internal void @libfun_
+
+! RUN1: not %flang_fc1 -emit-llvm -triple x86_64-unknown-linux-gnu -o - -mlink-builtin-bitcode %S/Inputs/no-bclib.bc %s 2>&1 | FileCheck %s
+
+! ERROR1: error: could not open {{.*}} no-bclib.bc
+
+external libfun
+parameter(i=1)
+integer :: j
+call libfun(j)
+end program

Leporacanthicus

Overall looks good. Minor nits on "binary files in repo" and duplication of code [although I couldn't find the corresponding bit in Clang - I didn't spend much time on searching for it].

Leporacanthicus · 2024-06-07T15:50:00Z

flang/test/Driver/Inputs/bclib.bc

Would it be possible to generate this, rather than supply it as a binary - just out of safety-concerns with binaries, really.

I expect the contents isn't particularly complex or time-consuming to build.

Yes, it should be possible to add a command to generate the .bc file, that is probably a better option anyway since we would avoid specifying the triple.

Leporacanthicus · 2024-06-07T15:53:12Z

flang/lib/Frontend/FrontendActions.cpp

@@ -1146,6 +1150,54 @@ void CodeGenAction::embedOffloadObjects() {
  }
 }

+void CodeGenAction::linkBuiltinBCLibs() {


This is a fairly large chunk of code - does it come from Clang in some way - if so, is it possible to have one common function shared, rather than two copies?

It does come from clang but it is distributed across a few classes with a lot more options, so there was not simple way to simply re-use the code. I don't know if we want to bring in all that code since it would add a lot more complexity, and I don't know if we need this in the future or not.

banach-space · 2024-06-09T13:51:24Z

flang/test/Driver/mlink-builtin-bc.f90

+
+! CHECK: define internal void @libfun_
+
+! RUN1: not %flang_fc1 -emit-llvm -triple x86_64-unknown-linux-gnu -o - -mlink-builtin-bitcode %S/Inputs/no-bclib.bc %s 2>&1 | FileCheck %s


What's RUN1?

My mistake, I was testing things out and it got in.

banach-space · 2024-06-09T13:51:32Z

flang/test/Driver/mlink-builtin-bc.f90

+
+! RUN1: not %flang_fc1 -emit-llvm -triple x86_64-unknown-linux-gnu -o - -mlink-builtin-bitcode %S/Inputs/no-bclib.bc %s 2>&1 | FileCheck %s
+
+! ERROR1: error: could not open {{.*}} no-bclib.bc


What's ERROR1?

banach-space · 2024-06-09T13:52:27Z

flang/test/Driver/mlink-builtin-bc.f90

+!----------
+! RUN lines
+!----------


In the past, folks asked not to add such comments in tests. Let's stick with that.

banach-space · 2024-06-09T13:53:28Z

flang/test/Driver/mlink-builtin-bc.f90

+! RUN lines
+!----------
+! Embed something that can be easily checked
+! RUN: %flang_fc1 -emit-llvm -triple x86_64-unknown-linux-gnu -o - -mlink-builtin-bitcode %S/Inputs/bclib.bc %s 2>&1 | FileCheck %s


What's bclib.bc and what makes it easy to check?

Does the triple matter?

The triple matters because when linking the data layout in bclib.bc must match the data layout of the compiled source code, and depending on the host it may not be the same so it is safest to specify the triple to ensure they are the same. However if the .bc file is generated also, then we can avoid specifying the triple.

banach-space · 2024-06-09T13:54:40Z

flang/lib/Frontend/FrontendActions.cpp

@@ -1146,6 +1150,54 @@ void CodeGenAction::embedOffloadObjects() {
  }
 }

+void CodeGenAction::linkBuiltinBCLibs() {


agozillon

LGTM, but please do wait on the other more knowledgeable reviewers approvals :-)

Leporacanthicus

Nit-pick, otherwise OK by me.

flang/test/Driver/mlink-builtin-bc.f90

Leporacanthicus

LGTM

flang/include/flang/Frontend/FrontendActions.h

Fix comment Co-authored-by: Sergio Afonso <safonsof@amd.com>

[flang] Add -mlink-builtin-bitcode option to fc1

adfeb69

This patch enables the -mlink-builtin-bitcode flag in fc1 so that bitcode libraries can be linked in. This is needed for OpenMP offloading libraries.

jsjodin requested review from lukel97, skatrak, banach-space, Leporacanthicus, agozillon and DominikAdamski June 7, 2024 15:37

llvmbot added clang Clang issues not falling into any other category flang:driver flang Flang issues not falling into any other category labels Jun 7, 2024

Leporacanthicus reviewed Jun 7, 2024

View reviewed changes

banach-space reviewed Jun 9, 2024

View reviewed changes

Use fortran source file to create bc library instead of using .bc file.

2b348b3

jsjodin requested review from banach-space and Leporacanthicus June 11, 2024 14:24

agozillon approved these changes Jun 11, 2024

View reviewed changes

Leporacanthicus reviewed Jun 11, 2024

View reviewed changes

flang/test/Driver/mlink-builtin-bc.f90 Outdated Show resolved Hide resolved

Remove usless triple flag

c4f5caf

jsjodin requested a review from Leporacanthicus June 12, 2024 14:02

Leporacanthicus approved these changes Jun 12, 2024

View reviewed changes

banach-space approved these changes Jun 12, 2024

View reviewed changes

skatrak reviewed Jun 13, 2024

View reviewed changes

flang/include/flang/Frontend/FrontendActions.h Outdated Show resolved Hide resolved

Update flang/include/flang/Frontend/FrontendActions.h

dac7c6f

Fix comment Co-authored-by: Sergio Afonso <safonsof@amd.com>

jsjodin merged commit b75e7c6 into llvm:main Jun 17, 2024
7 checks passed

jpienaar mentioned this pull request Jun 17, 2024

[mlirc] Add missing extern C #95829

Merged

jsjodin deleted the jleyonberg/flangbuiltinbclibs branch June 25, 2024 13:56

DominikAdamski mentioned this pull request Jul 1, 2024

[Flang-new][OpenMP] Add bitcode files for AMD GPU OpenMP #96742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang] Add -mlink-builtin-bitcode option to fc1 #94763

[flang] Add -mlink-builtin-bitcode option to fc1 #94763

jsjodin commented Jun 7, 2024

llvmbot commented Jun 7, 2024 •

edited

Loading

Leporacanthicus left a comment

Leporacanthicus Jun 7, 2024

jsjodin Jun 10, 2024

Leporacanthicus Jun 7, 2024

banach-space Jun 9, 2024

jsjodin Jun 10, 2024

banach-space Jun 9, 2024

jsjodin Jun 11, 2024

banach-space Jun 9, 2024

jsjodin Jun 11, 2024

banach-space Jun 9, 2024

banach-space Jun 9, 2024

jsjodin Jun 10, 2024 •

edited

Loading

banach-space Jun 9, 2024

agozillon left a comment

Leporacanthicus left a comment

Leporacanthicus left a comment


		! CHECK: define internal void @libfun_

		! RUN1: not %flang_fc1 -emit-llvm -triple x86_64-unknown-linux-gnu -o - -mlink-builtin-bitcode %S/Inputs/no-bclib.bc %s 2>&1 \| FileCheck %s


		! RUN1: not %flang_fc1 -emit-llvm -triple x86_64-unknown-linux-gnu -o - -mlink-builtin-bitcode %S/Inputs/no-bclib.bc %s 2>&1 \| FileCheck %s

		! ERROR1: error: could not open {{.*}} no-bclib.bc

[flang] Add -mlink-builtin-bitcode option to fc1 #94763

[flang] Add -mlink-builtin-bitcode option to fc1 #94763

Conversation

jsjodin commented Jun 7, 2024

llvmbot commented Jun 7, 2024 • edited Loading

Leporacanthicus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjodin Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agozillon left a comment

Choose a reason for hiding this comment

Leporacanthicus left a comment

Choose a reason for hiding this comment

Leporacanthicus left a comment

Choose a reason for hiding this comment

llvmbot commented Jun 7, 2024 •

edited

Loading

jsjodin Jun 10, 2024 •

edited

Loading