[mlir] Add llvm.linker.options operation to the LLVM IR Dialect #71720

DavidTruby · 2023-11-08T18:39:29Z

This patch adds a llvm.linker.options operation taking a list of
strings to pass to the linker when the resulting object file is linked.
This is particularly useful on Windows to specify the CRT version to use
for this object file.

This patch adds a `llvm.linker.options` operation taking a list of strings to pass to the linker when the resulting object file is linked. This is particularly useful on Windows to specify the CRT version to use for this object file.

llvmbot · 2023-11-08T18:39:57Z

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: David Truby (DavidTruby)

Changes

This patch adds a llvm.linker.options operation taking a list of
strings to pass to the linker when the resulting object file is linked.
This is particularly useful on Windows to specify the CRT version to use
for this object file.

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

5 Files Affected:

(modified) mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td (+38)
(modified) mlir/include/mlir/Target/LLVMIR/ModuleImport.h (+4)
(modified) mlir/lib/Target/LLVMIR/ModuleImport.cpp (+23)
(added) mlir/test/Target/LLVMIR/Import/metadata-linker-options.ll (+15)
(modified) mlir/test/Target/LLVMIR/llvmir.mlir (+6)

diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index 638c31b39682ea6..7635bc92be0e136 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -1825,4 +1825,42 @@ def LLVM_CallIntrinsicOp
   let hasVerifier = 1;
 }
 
+def LLVM_LinkerOptionsOp
+    : LLVM_Op<"linker.options"> {
+  let summary = "Options to pass to the linker when the object file is linked";
+  let description = [{
+    Pass the given options to the linker when the resulting object file is linked.
+    This is used extensively on Windows to determine the C runtime that the object
+    files should link against.
+
+    Examples:
+    ```mlir
+    // Link against the MSVC static threaded CRT.
+    llvm.linker.options ["/DEFAULTLIB:", "libcmt"]
+
+    // Link against aarch64 compiler-rt builtins
+    llvm.linker.options ["-l", "clang_rt.builtins-aarch64"]
+    ```
+  }];
+  let arguments  = (ins StrArrayAttr:$options);
+  let assemblyFormat = [{
+    $options attr-dict
+  }];
+
+  let llvmBuilder = [{
+    llvm::Module *llvmModule = moduleTranslation.getLLVMModule();
+    llvm::LLVMContext &context = llvmModule->getContext();
+    llvm::NamedMDNode *linkerMDNode = llvmModule->getOrInsertNamedMetadata("llvm.linker.options");
+    SmallVector<llvm::Metadata*> MDNodes;
+    for (auto s : $options) {
+      auto str = cast<StringAttr>(s);
+      auto *MDNode = llvm::MDString::get(context, str.getValue());
+      MDNodes.push_back(MDNode);
+    }
+
+    auto *listMDNode = llvm::MDTuple::get(context, MDNodes);
+    linkerMDNode->addOperand(listMDNode);
+  }];
+}
+
 #endif // LLVMIR_OPS
diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
index 9bedc84e0bfa169..5f5adbff6c04ef8 100644
--- a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
+++ b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
@@ -177,6 +177,10 @@ class ModuleImport {
   /// implement the fastmath interface.
   void setFastmathFlagsAttr(llvm::Instruction *inst, Operation *op) const;
 
+  /// Converts !llvm.linker.options metadata to the llvm.linker.options
+  /// LLVM dialect operation.
+  LogicalResult convertLinkerOptionsMetadata();
+
   /// Converts all LLVM metadata nodes that translate to attributes such as
   /// alias analysis or access group metadata, and builds a map from the
   /// metadata nodes to the converted attributes.
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index e3562049cd81c76..7d133b309f87a7c 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -487,6 +487,27 @@ void ModuleImport::addDebugIntrinsic(llvm::CallInst *intrinsic) {
   debugIntrinsics.insert(intrinsic);
 }
 
+LogicalResult ModuleImport::convertLinkerOptionsMetadata() {
+  for (const llvm::NamedMDNode &named : llvmModule->named_metadata()) {
+    if (named.getName() != "llvm.linker.options")
+      continue;
+    // llvm.linker.options operands are lists of strings.
+    for (const llvm::MDNode *md : named.operands()) {
+      SmallVector<StringRef> options;
+      for (const llvm::MDOperand &option : md->operands()) {
+        if (auto str = dyn_cast_or_null<llvm::MDString>(option))
+          options.push_back(str->getString());
+        else
+          return emitError(mlirModule.getLoc(),
+                           "argument to llvm.linker.options is not a string");
+      }
+      builder.create<LLVM::LinkerOptionsOp>(mlirModule.getLoc(),
+                                            builder.getStrArrayAttr(options));
+    }
+  }
+  return success();
+}
+
 LogicalResult ModuleImport::convertMetadata() {
   OpBuilder::InsertionGuard guard(builder);
   builder.setInsertionPointToEnd(mlirModule.getBody());
@@ -513,6 +534,8 @@ LogicalResult ModuleImport::convertMetadata() {
           return failure();
     }
   }
+  if (failed(convertLinkerOptionsMetadata()))
+    return failure();
   return success();
 }
 
diff --git a/mlir/test/Target/LLVMIR/Import/metadata-linker-options.ll b/mlir/test/Target/LLVMIR/Import/metadata-linker-options.ll
new file mode 100644
index 000000000000000..473d26ecd598959
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/Import/metadata-linker-options.ll
@@ -0,0 +1,15 @@
+; RUN: mlir-translate -import-llvm -split-input-file %s | FileCheck %s
+
+; CHECK: llvm.linker.options ["DEFAULTLIB:", "libcmt"]
+!llvm.linker.options = !{!0}
+!0 = !{!"DEFAULTLIB:", !"libcmt"}
+
+; // -----
+
+!llvm.linker.options = !{!0, !1, !2}
+; CHECK: llvm.linker.options ["DEFAULTLIB:", "libcmt"]
+!0 = !{!"DEFAULTLIB:", !"libcmt"}
+; CHECK: llvm.linker.options ["DEFAULTLIB:", "libcmtd"]
+!1 = !{!"DEFAULTLIB:", !"libcmtd"}
+; CHECK: llvm.linker.options ["-lm"]
+!2 = !{!"-lm"}
diff --git a/mlir/test/Target/LLVMIR/llvmir.mlir b/mlir/test/Target/LLVMIR/llvmir.mlir
index 7da44b6fbe1ab33..fa3cd7679112440 100644
--- a/mlir/test/Target/LLVMIR/llvmir.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir.mlir
@@ -2337,3 +2337,9 @@ llvm.func @zeroinit_complex_local_aggregate() {
 
   llvm.return
 }
+
+//CHECK: !llvm.linker.options = !{![[MD0:[0-9]+]], ![[MD1:[0-9]+]]}
+//CHECK: ![[MD0]] = !{!"/DEFAULTLIB:", !"libcmt"}
+llvm.linker.options ["/DEFAULTLIB:", "libcmt"]
+//CHECK: ![[MD1]] = !{!"/DEFAULTLIB:", !"libcmtd"}
+llvm.linker.options ["/DEFAULTLIB:", "libcmtd"]
\ No newline at end of file

Dinistro

Seems useful and a valid addition, given that LLVM has a matching construct.
Could you also add a minimal dialect roundtrip test somewhere in mlir/test/Dialect/LLVMIR/?

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

ftynse

LGTM in principle with comments addressed.

ftynse · 2023-11-09T08:38:45Z

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

+    llvm::Module *llvmModule = moduleTranslation.getLLVMModule();
+    llvm::LLVMContext &context = llvmModule->getContext();
+    llvm::NamedMDNode *linkerMDNode = llvmModule->getOrInsertNamedMetadata("llvm.linker.options");
+    SmallVector<llvm::Metadata*> MDNodes;
+    for (auto s : $options) {
+      auto str = cast<StringAttr>(s);
+      auto *MDNode = llvm::MDString::get(context, str.getValue());
+      MDNodes.push_back(MDNode);
+    }
+
+    auto *listMDNode = llvm::MDTuple::get(context, MDNodes);
+    linkerMDNode->addOperand(listMDNode);


Nit: can we put this into a static function that we just call here? C++ embedded as strings into Tablegen is quite difficult to maintain.

ftynse · 2023-11-09T08:39:26Z

mlir/lib/Target/LLVMIR/ModuleImport.cpp

+    for (const llvm::MDNode *md : named.operands()) {
+      SmallVector<StringRef> options;
+      for (const llvm::MDOperand &option : md->operands()) {
+        if (auto str = dyn_cast_or_null<llvm::MDString>(option))


Can it ever be null?

Not if the LLVM IR is valid; I wasn’t sure if it was verified another way before going through the import or if I should do it here. I can change this to just a cast if that’s the case.

You can assume verified LLVM IR in the import.

ftynse · 2023-11-09T08:39:45Z

mlir/test/Target/LLVMIR/llvmir.mlir

+//CHECK: ![[MD0]] = !{!"/DEFAULTLIB:", !"libcmt"}
+llvm.linker.options ["/DEFAULTLIB:", "libcmt"]
+//CHECK: ![[MD1]] = !{!"/DEFAULTLIB:", !"libcmtd"}
+llvm.linker.options ["/DEFAULTLIB:", "libcmtd"]


Nit: please add trailing newline.

mlir/test/Target/LLVMIR/llvmir.mlir

* Change name of operation to linker_options * Add verifier that the operation can only appear in modules * Move C++ code into a static function

github-actions · 2023-11-09T16:06:59Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Dinistro

Some additional comments

Dinistro · 2023-11-09T15:57:02Z

mlir/test/Target/LLVMIR/llvmir-invalid.mlir

+llvm.func @foo() {
+  // expected-error @below{{must appear at the module level}}
+  llvm.linker_options ["test"]
+}


Nit: Lacks a trailing newline.

Dinistro · 2023-11-09T17:04:48Z

mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp

+  llvm::NamedMDNode *linkerMDNode =
+      llvmModule->getOrInsertNamedMetadata("llvm.linker.options");
+  SmallVector<llvm::Metadata *> MDNodes;
+  for (auto s : options) {


Nit: You should be able to use options.getAsRange<StringAttr>() here, to avoid the cast.

mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp

Dinistro · 2023-11-09T17:05:45Z

mlir/lib/Target/LLVMIR/ModuleImport.cpp

+      for (const llvm::MDOperand &option : md->operands()) {
+        options.push_back(cast<llvm::MDString>(option)->getString());
+      }


Suggested change

for (const llvm::MDOperand &option : md->operands()) {

options.push_back(cast<llvm::MDString>(option)->getString());

}

for (const llvm::MDOperand &option : md->operands())

options.push_back(cast<llvm::MDString>(option)->getString());

mlir/lib/Target/LLVMIR/ModuleImport.cpp

Dinistro

LGTM, but please wait for the @joker-eph to take a look at your answer.

joker-eph · 2023-11-11T02:17:53Z

LG

…#71720) This patch adds a `llvm.linker.options` operation taking a list of strings to pass to the linker when the resulting object file is linked. This is particularly useful on Windows to specify the CRT version to use for this object file.

llvmbot added mlir:llvm mlir labels Nov 8, 2023

DavidTruby requested review from gysit and ftynse November 8, 2023 18:39

DavidTruby mentioned this pull request Nov 8, 2023

[flang][windows] Add option to link against specific MSVC CRT #70833

Merged

Dinistro reviewed Nov 9, 2023

View reviewed changes

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td Outdated Show resolved Hide resolved

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td Show resolved Hide resolved

ftynse reviewed Nov 9, 2023

View reviewed changes

joker-eph reviewed Nov 9, 2023

View reviewed changes

mlir/test/Target/LLVMIR/llvmir.mlir Outdated Show resolved Hide resolved

DavidTruby added 2 commits November 9, 2023 15:53

Fixes for review

bf0f720

* Change name of operation to linker_options * Add verifier that the operation can only appear in modules * Move C++ code into a static function

Assume verified IR

a42a125

clang-format fixes

29f1c74

Dinistro reviewed Nov 9, 2023

View reviewed changes

Reserve vector sizes

e7e03db

Dinistro approved these changes Nov 10, 2023

View reviewed changes

ftynse approved these changes Nov 13, 2023

View reviewed changes

DavidTruby merged commit a72e034 into llvm:main Nov 13, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mlir] Add llvm.linker.options operation to the LLVM IR Dialect #71720

[mlir] Add llvm.linker.options operation to the LLVM IR Dialect #71720

DavidTruby commented Nov 8, 2023

llvmbot commented Nov 8, 2023 •

edited

Loading

Dinistro left a comment

ftynse left a comment

ftynse Nov 9, 2023

ftynse Nov 9, 2023

DavidTruby Nov 9, 2023

Dinistro Nov 9, 2023

ftynse Nov 9, 2023

github-actions bot commented Nov 9, 2023 •

edited

Loading

Dinistro left a comment

Dinistro Nov 9, 2023

Dinistro Nov 9, 2023

Dinistro Nov 9, 2023

Dinistro left a comment

joker-eph commented Nov 11, 2023

[mlir] Add llvm.linker.options operation to the LLVM IR Dialect #71720

[mlir] Add llvm.linker.options operation to the LLVM IR Dialect #71720

Conversation

DavidTruby commented Nov 8, 2023

llvmbot commented Nov 8, 2023 • edited Loading

Dinistro left a comment

Choose a reason for hiding this comment

ftynse 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

github-actions bot commented Nov 9, 2023 • edited Loading

Dinistro 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

Dinistro left a comment

Choose a reason for hiding this comment

joker-eph commented Nov 11, 2023

llvmbot commented Nov 8, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading