[flang] Add dependent-lib option to flang -fc1 on Windows #72121

DavidTruby · 2023-11-13T14:53:29Z

This patch adds a --dependent-lib option to flang -fc1 on Windows to
embed library link options into the object file. This is needed to
properly select the Windows CRT to link against.

This patch adds a --depdendent-lib option to flang -fc1 on Windows to embed library link options into the object file. This is needed to properly select the Windows CRT to link against.

llvmbot · 2023-11-13T14:54:04Z

@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang

Author: David Truby (DavidTruby)

Changes

This patch adds a --depdendent-lib option to flang -fc1 on Windows to
embed library link options into the object file. This is needed to
properly select the Windows CRT to link against.

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

6 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+5-3)
(modified) flang/include/flang/Frontend/CodeGenOptions.h (+3)
(modified) flang/lib/Frontend/CompilerInvocation.cpp (+23)
(modified) flang/lib/Frontend/FrontendActions.cpp (+23)
(added) flang/test/Driver/dependent-lib.f90 (+7)
(modified) flang/test/Driver/driver-help.f90 (+1)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index d1b67a448b2a59b..c7eb52145f614b4 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -6796,9 +6796,6 @@ def vectorize_loops : Flag<["-"], "vectorize-loops">,
 def vectorize_slp : Flag<["-"], "vectorize-slp">,
   HelpText<"Run the SLP vectorization passes">,
   MarshallingInfoFlag<CodeGenOpts<"VectorizeSLP">>;
-def dependent_lib : Joined<["--"], "dependent-lib=">,
-  HelpText<"Add dependent library">,
-  MarshallingInfoStringVector<CodeGenOpts<"DependentLibraries">>;
 def linker_option : Joined<["--"], "linker-option=">,
   HelpText<"Add linker option">,
   MarshallingInfoStringVector<CodeGenOpts<"LinkerOptions">>;
@@ -7369,6 +7366,11 @@ def pic_is_pie : Flag<["-"], "pic-is-pie">,
   HelpText<"File is for a position independent executable">,
   MarshallingInfoFlag<LangOpts<"PIE">>;
 
+
+def dependent_lib : Joined<["--"], "dependent-lib=">,
+  HelpText<"Add dependent library">,
+  MarshallingInfoStringVector<CodeGenOpts<"DependentLibraries">>;
+
 } // let Visibility = [CC1Option, FC1Option]
 
 let Visibility = [CC1Option] in {
diff --git a/flang/include/flang/Frontend/CodeGenOptions.h b/flang/include/flang/Frontend/CodeGenOptions.h
index a3c39bda10667be..b86bb88610a9a4a 100644
--- a/flang/include/flang/Frontend/CodeGenOptions.h
+++ b/flang/include/flang/Frontend/CodeGenOptions.h
@@ -70,6 +70,9 @@ class CodeGenOptions : public CodeGenOptionsBase {
   /// The format used for serializing remarks (default: YAML)
   std::string OptRecordFormat;
 
+  /// Options to add to the linker for the object file
+  std::vector<std::string> DependentLibs;
+
   // The RemarkKind enum class and OptRemark struct are identical to what Clang
   // has
   // TODO: Share with clang instead of re-implementing here
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index 334da3ac287e3bf..cb4f2d6a6225205 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -1054,6 +1054,27 @@ static bool parseVScaleArgs(CompilerInvocation &invoc, llvm::opt::ArgList &args,
   return true;
 }
 
+static bool parseLinkerOptionsArgs(CompilerInvocation &invoc,
+                                   llvm::opt::ArgList &args,
+                                   clang::DiagnosticsEngine &diags) {
+  llvm::Triple triple = llvm::Triple(invoc.getTargetOpts().triple);
+
+  // TODO: support --dependent-lib on other platforms when MLIR supports
+  //       !llvm.dependent.lib
+  if (args.hasArg(clang::driver::options::OPT_dependent_lib) &&
+      !triple.isOSWindows()) {
+    const unsigned diagID =
+        diags.getCustomDiagID(clang::DiagnosticsEngine::Error,
+                              "--dependent-lib is only supported on Windows");
+    diags.Report(diagID);
+    return false;
+  }
+
+  invoc.getCodeGenOpts().DependentLibs =
+      args.getAllArgValues(clang::driver::options::OPT_dependent_lib);
+  return true;
+}
+
 bool CompilerInvocation::createFromArgs(
     CompilerInvocation &res, llvm::ArrayRef<const char *> commandLineArgs,
     clang::DiagnosticsEngine &diags, const char *argv0) {
@@ -1163,6 +1184,8 @@ bool CompilerInvocation::createFromArgs(
 
   success &= parseVScaleArgs(res, args, diags);
 
+  success &= parseLinkerOptionsArgs(res, args, diags);
+
   // Set the string to be used as the return value of the COMPILER_OPTIONS
   // intrinsic of iso_fortran_env. This is either passed in from the parent
   // compiler driver invocation with an environment variable, or failing that
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index d7ca7b66584dd52..8df966571bc39ab 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -203,6 +203,26 @@ static void setMLIRDataLayout(mlir::ModuleOp &mlirModule,
   mlirModule->setAttr(mlir::DLTIDialect::kDataLayoutAttrName, dlSpec);
 }
 
+static void addDepdendentLibs(mlir::ModuleOp &mlirModule,
+                              CompilerInstance &ci) {
+  const std::vector<std::string> &libs =
+      ci.getInvocation().getCodeGenOpts().DependentLibs;
+  if (libs.empty()) {
+    return;
+  }
+  // dependent-lib is currently only supported on Windows, so the list should be
+  // empty on non-Windows platforms
+  assert(
+      llvm::Triple(ci.getInvocation().getTargetOpts().triple).isOSWindows() &&
+      "--dependent-lib is only supported on Windows");
+  // Add linker options specified by --dependent-lib
+  auto builder = mlir::OpBuilder(mlirModule.getRegion());
+  for (const std::string &lib : libs) {
+    builder.create<mlir::LLVM::LinkerOptionsOp>(
+        mlirModule.getLoc(), builder.getStrArrayAttr({"/DEFAULTLIB:", lib}));
+  }
+}
+
 bool CodeGenAction::beginSourceFileAction() {
   llvmCtx = std::make_unique<llvm::LLVMContext>();
   CompilerInstance &ci = this->getInstance();
@@ -303,6 +323,9 @@ bool CodeGenAction::beginSourceFileAction() {
   Fortran::parser::Program &parseTree{*ci.getParsing().parseTree()};
   lb.lower(parseTree, ci.getInvocation().getSemanticsContext());
 
+  // Add dependent libraries
+  addDepdendentLibs(*mlirModule, ci);
+
   // run the default passes.
   mlir::PassManager pm((*mlirModule)->getName(),
                        mlir::OpPassManager::Nesting::Implicit);
diff --git a/flang/test/Driver/dependent-lib.f90 b/flang/test/Driver/dependent-lib.f90
new file mode 100644
index 000000000000000..e455d06258662a1
--- /dev/null
+++ b/flang/test/Driver/dependent-lib.f90
@@ -0,0 +1,7 @@
+
+! RUN: %flang_fc1 -emit-mlir -triple aarch64-pc-windows-msvc --dependent-lib=libtest %s -o - 2>&1 | FileCheck %s
+! RUN: %flang_fc1 -emit-mlir -triple x86_64-pc-windows-msvc --dependent-lib=libtest %s -o - 2>&1 | FileCheck %s
+
+! CHECK: llvm.linker_options ["/DEFAULTLIB:", "libtest"]
+program test
+end program test
\ No newline at end of file
diff --git a/flang/test/Driver/driver-help.f90 b/flang/test/Driver/driver-help.f90
index ea9c892bd621058..452c62541e72e61 100644
--- a/flang/test/Driver/driver-help.f90
+++ b/flang/test/Driver/driver-help.f90
@@ -141,6 +141,7 @@
 ! HELP-FC1-EMPTY:
 ! HELP-FC1-NEXT:OPTIONS:
 ! HELP-FC1-NEXT: -cpp                    Enable predefined and command line preprocessor macros
+! HELP-FC1-NEXT: --dependent-lib=<value> Add dependent library
 ! HELP-FC1-NEXT: -D <macro>=<value>      Define <macro> to <value> (or 1 if <value> omitted)
 ! HELP-FC1-NEXT: -emit-fir               Build the parse tree, then lower it to FIR
 ! HELP-FC1-NEXT: -emit-hlfir             Build the parse tree, then lower it to HLFIR

banach-space

Great to see contributions for supporting Windows, thank you David!

banach-space · 2023-11-13T22:34:44Z

flang/test/Driver/dependent-lib.f90

+
+! CHECK: llvm.linker_options ["/DEFAULTLIB:", "libtest"]
+program test
+end program test


Missing EOF?

banach-space · 2023-11-13T22:34:57Z

flang/test/Driver/dependent-lib.f90

@@ -0,0 +1,7 @@
+


[nit] This line can be skipped?

flang/lib/Frontend/CompilerInvocation.cpp

banach-space · 2023-11-13T22:38:16Z

flang/lib/Frontend/FrontendActions.cpp

+  // Add dependent libraries
+  addDepdendentLibs(*mlirModule, ci);


What are the semantics of "dependent-lib" - what's the consumer of this flag? Wondering whether this isn't too early in the compilation to call this hook.

The consumer is that an LLVMIR dialect operation should be added to the MLIR module (in the global module scope) that will eventually get lowered by the standard MLIR LLVM dialect to LLVM ir transform. I don’t think it particularly matters where in the compilation flow that gets added as it doesn’t affect or get affected by any other transformation. I’m happy to add it later if you have a suggestion where it should be done, I mostly did it here because you still have all the information to construct the list of linker options without needing to pass any information further on.

Apologies, I thought that this was FrontendAction::beginSourceFile rather than CodeGenAction::beginSourceFileAction().

I think that keeping it here is fine. Like you said, the ordering in this case doesn't matter that much.

banach-space · 2023-11-13T22:39:09Z

flang/test/Driver/dependent-lib.f90

+! RUN: %flang_fc1 -emit-mlir -triple aarch64-pc-windows-msvc --dependent-lib=libtest %s -o - 2>&1 | FileCheck %s
+! RUN: %flang_fc1 -emit-mlir -triple x86_64-pc-windows-msvc --dependent-lib=libtest %s -o - 2>&1 | FileCheck %s


[nit] If you only want to check the triple, then you could use LIT's DEFINE/REDEFINE here.

I changed to use this but I'm not sure it's actually any clearer than just listing the run lines. Let me know what you think now that I've put it in 👍

tblah

LGTM with Andrzej's comments

banach-space

LGTM :)

Thank you for addressing my comments.

banach-space · 2023-11-15T08:14:09Z

flang/test/Driver/dependent-lib.f90

+! DEFINE: %{triple} =
+! DEFINE: %{run} = %flang_fc1 -emit-mlir -triple %{triple} --dependent-lib=libtest %s -o - 2>&1 | FileCheck %s
+! REDEFINE: %{triple} = aarch64-pc-windows-msvc
+! RUN: %{run}
+! REDEFINE: %{triple} = x86_64-pc-windows-msvc
+! RUN: %{run}
+! REDEFINE: %{triple} = x86_64-linux-unknown-gnu
+! RUN: not %{run} --check-prefixes=CHECK-NOWIN
+! REDEFINE: %{triple} = aarch64-apple-darwin
+! RUN: not %{run} --check-prefixes=CHECK-NOWIN


I see what you meant about readability - indeed debatable 😅 . Here's an idea how to improve it a bit:

Suggested change

! DEFINE: %{triple} =

! DEFINE: %{run} = %flang_fc1 -emit-mlir -triple %{triple} --dependent-lib=libtest %s -o - 2>&1 | FileCheck %s

! REDEFINE: %{triple} = aarch64-pc-windows-msvc

! RUN: %{run}

! REDEFINE: %{triple} = x86_64-pc-windows-msvc

! RUN: %{run}

! REDEFINE: %{triple} = x86_64-linux-unknown-gnu

! RUN: not %{run} --check-prefixes=CHECK-NOWIN

! REDEFINE: %{triple} = aarch64-apple-darwin

! RUN: not %{run} --check-prefixes=CHECK-NOWIN

! DEFINE: %{triple} =

! DEFINE: %{compile} = %flang_fc1 -emit-mlir -triple %{triple} --dependent-lib=libtest %s -o - 2>&1

! REDEFINE: %{triple} = aarch64-pc-windows-msvc

! RUN: %{compile} | FileCheck %s

! REDEFINE: %{triple} = x86_64-pc-windows-msvc

! RUN: %{compile} | FileCheck %s

! REDEFINE: %{triple} = x86_64-linux-unknown-gnu

! RUN: not %{compile} | FileCheck %s --check-prefixes=CHECK-NOWIN

! REDEFINE: %{triple} = aarch64-apple-darwin

! RUN: not %{compile} | FileCheck %s --check-prefixes=CHECK-NOWIN

(separating the compiler invocation from the FileCheck invocation as two logically seperate things).

This is a "nit" - I am happy with this test as long as the functionality is verified (it is).

banach-space · 2023-11-15T08:23:51Z

flang/lib/Frontend/FrontendActions.cpp

+  // Add dependent libraries
+  addDepdendentLibs(*mlirModule, ci);


Apologies, I thought that this was FrontendAction::beginSourceFile rather than CodeGenAction::beginSourceFileAction().

I think that keeping it here is fine. Like you said, the ordering in this case doesn't matter that much.

antmox · 2023-11-15T15:55:00Z

hello! looks like this broke some bots:
https://lab.llvm.org/buildbot/#/builders/175/builds/39052
https://lab.llvm.org/buildbot/#/builders/268/builds/2443
could it be the missing colon after REQUIRES in dependent-lib.f90 ?

This patch adds a --dependent-lib option to flang -fc1 on Windows to embed library link options into the object file. This is needed to properly select the Windows CRT to link against.

[flang] Add depdendent-lib option to flang -fc1 on Windows

cfd46a8

This patch adds a --depdendent-lib option to flang -fc1 on Windows to embed library link options into the object file. This is needed to properly select the Windows CRT to link against.

DavidTruby requested review from banach-space, rnk and zmodem November 13, 2023 14:53

llvmbot added clang Clang issues not falling into any other category flang:driver flang Flang issues not falling into any other category labels Nov 13, 2023

DavidTruby requested review from Leporacanthicus and tblah November 13, 2023 14:53

banach-space reviewed Nov 13, 2023

View reviewed changes

tblah approved these changes Nov 14, 2023

View reviewed changes

Add test for diagnostic on non-Windows

4bd30c5

banach-space approved these changes Nov 15, 2023

View reviewed changes

Fix up test

506020d

DavidTruby changed the title ~~[flang] Add depdendent-lib option to flang -fc1 on Windows~~ [flang] Add dependent-lib option to flang -fc1 on Windows Nov 15, 2023

DavidTruby merged commit 77ecb9a into llvm:main Nov 15, 2023
2 of 3 checks passed

antmox mentioned this pull request Nov 15, 2023

[flang] fix test dependent-lib.f90 #72409

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang] Add dependent-lib option to flang -fc1 on Windows #72121

[flang] Add dependent-lib option to flang -fc1 on Windows #72121

DavidTruby commented Nov 13, 2023 •

edited

llvmbot commented Nov 13, 2023 •

edited

banach-space left a comment

banach-space Nov 13, 2023

banach-space Nov 13, 2023

banach-space Nov 13, 2023

DavidTruby Nov 14, 2023

banach-space Nov 15, 2023

banach-space Nov 13, 2023

DavidTruby Nov 14, 2023

tblah left a comment

banach-space left a comment

banach-space Nov 15, 2023

banach-space Nov 15, 2023

antmox commented Nov 15, 2023

		// Add dependent libraries
		addDepdendentLibs(*mlirModule, ci);

		! RUN: %flang_fc1 -emit-mlir -triple aarch64-pc-windows-msvc --dependent-lib=libtest %s -o - 2>&1 \| FileCheck %s
		! RUN: %flang_fc1 -emit-mlir -triple x86_64-pc-windows-msvc --dependent-lib=libtest %s -o - 2>&1 \| FileCheck %s

[flang] Add dependent-lib option to flang -fc1 on Windows #72121

[flang] Add dependent-lib option to flang -fc1 on Windows #72121

Conversation

DavidTruby commented Nov 13, 2023 • edited

llvmbot commented Nov 13, 2023 • edited

banach-space 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

tblah left a comment

Choose a reason for hiding this comment

banach-space left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antmox commented Nov 15, 2023

DavidTruby commented Nov 13, 2023 •

edited

llvmbot commented Nov 13, 2023 •

edited