Skip to content

Conversation

VadimCurca
Copy link
Contributor

@VadimCurca VadimCurca commented Aug 21, 2025

Adds a target_specific_attrs optional array attribute to mlir.global, as well as conversions to and from LLVM attributes on llvm::GlobalVariable objects. This is necessary to preserve unknown attributes on global variables when converting to and from the LLVM Dialect. Previously, any attributes on an llvm::GlobalVariable not explicitly modeled by mlir.global were dropped during conversion.

@@ -1395,6 +1395,53 @@ LogicalResult ModuleImport::convertIFunc(llvm::GlobalIFunc *ifunc) {
return success();
}

/// Converts LLVM attributes from `globalVar` into MLIR attributes and adds them
/// to `globalOp` as passthrough attributes.
static void processPassthroughAttrs(llvm::GlobalVariable *globalVar,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this partially be shared with the one for functions?

@VadimCurca VadimCurca force-pushed the vadimc/add_globalOp_passthrough branch from aaa040d to 4d325a0 Compare August 22, 2025 06:53
@VadimCurca VadimCurca requested a review from Dinistro August 22, 2025 06:55
@VadimCurca VadimCurca marked this pull request as ready for review August 22, 2025 08:52
@llvmbot
Copy link
Member

llvmbot commented Aug 22, 2025

@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-llvm-ir

Author: Vadim Curcă (VadimCurca)

Changes

Adds a passthrough optional array attribute to mlir.global, as well as conversions to and from LLVM attributes on llvm::GlobalVariable objects. This is necessary to preserve unknown attributes on global variables when converting to and from the LLVM Dialect. Previously, any attributes on an llvm::GlobalVariable not explicitly modeled by mlir.global were dropped during conversion.


Patch is 24.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/154706.diff

10 Files Affected:

  • (modified) llvm/include/llvm/IR/Attributes.h (+5)
  • (modified) llvm/include/llvm/IR/GlobalVariable.h (+5)
  • (modified) llvm/lib/IR/Attributes.cpp (+6)
  • (modified) mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td (+2-1)
  • (modified) mlir/lib/Target/LLVMIR/ModuleImport.cpp (+73-53)
  • (modified) mlir/lib/Target/LLVMIR/ModuleTranslation.cpp (+96-78)
  • (modified) mlir/test/Target/LLVMIR/Import/function-attributes.ll (+5)
  • (modified) mlir/test/Target/LLVMIR/Import/global-variables.ll (+24-12)
  • (modified) mlir/test/Target/LLVMIR/llvmir-invalid.mlir (+20)
  • (modified) mlir/test/Target/LLVMIR/llvmir.mlir (+12)
diff --git a/llvm/include/llvm/IR/Attributes.h b/llvm/include/llvm/IR/Attributes.h
index 2b500ed58d0d6..9f01c76e6be48 100644
--- a/llvm/include/llvm/IR/Attributes.h
+++ b/llvm/include/llvm/IR/Attributes.h
@@ -384,6 +384,11 @@ class AttributeSet {
   bool operator==(const AttributeSet &O) const { return SetNode == O.SetNode; }
   bool operator!=(const AttributeSet &O) const { return !(*this == O); }
 
+  /// Add an attribute to the attribute set. Returns a new set because attribute
+  /// sets are immutable.
+  [[nodiscard]] AttributeSet LLVM_ABI addAttribute(LLVMContext &C,
+                                                   Attribute A) const;
+
   /// Add an argument attribute. Returns a new set because attribute sets are
   /// immutable.
   [[nodiscard]] LLVM_ABI AttributeSet
diff --git a/llvm/include/llvm/IR/GlobalVariable.h b/llvm/include/llvm/IR/GlobalVariable.h
index 388e1d7cfa808..3acd60e397225 100644
--- a/llvm/include/llvm/IR/GlobalVariable.h
+++ b/llvm/include/llvm/IR/GlobalVariable.h
@@ -209,6 +209,11 @@ class GlobalVariable : public GlobalObject, public ilist_node<GlobalVariable> {
   LLVM_ABI void
   getDebugInfo(SmallVectorImpl<DIGlobalVariableExpression *> &GVs) const;
 
+  /// Add attribute to this global.
+  void addAttribute(Attribute Attr) {
+    Attrs = Attrs.addAttribute(getContext(), Attr);
+  }
+
   /// Add attribute to this global.
   void addAttribute(Attribute::AttrKind Kind) {
     Attrs = Attrs.addAttribute(getContext(), Kind);
diff --git a/llvm/lib/IR/Attributes.cpp b/llvm/lib/IR/Attributes.cpp
index d1fbcb9e893a7..6ee78e1cab365 100644
--- a/llvm/lib/IR/Attributes.cpp
+++ b/llvm/lib/IR/Attributes.cpp
@@ -934,6 +934,12 @@ AttributeSet AttributeSet::addAttribute(LLVMContext &C,
   return addAttributes(C, AttributeSet::get(C, B));
 }
 
+AttributeSet AttributeSet::addAttribute(LLVMContext &C, Attribute A) const {
+  AttrBuilder B(C);
+  B.addAttribute(A);
+  return addAttributes(C, AttributeSet::get(C, B));
+}
+
 AttributeSet AttributeSet::addAttribute(LLVMContext &C, StringRef Kind,
                                         StringRef Value) const {
   AttrBuilder B(C);
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index 3f27f6d9ae8b7..8ebc2d0cbea77 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -1313,7 +1313,8 @@ def LLVM_GlobalOp : LLVM_Op<"mlir.global",
     OptionalAttr<StrAttr>:$section,
     OptionalAttr<SymbolRefAttr>:$comdat,
     OptionalAttr<DIGlobalVariableExpressionArrayAttr>:$dbg_exprs,
-    DefaultValuedAttr<Visibility, "mlir::LLVM::Visibility::Default">:$visibility_
+    DefaultValuedAttr<Visibility, "mlir::LLVM::Visibility::Default">:$visibility_,
+    OptionalAttr<ArrayAttr>:$passthrough
   );
   let summary = "LLVM dialect global.";
   let description = [{
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index a207cceef88b5..12275fb6a3abf 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -1395,6 +1395,67 @@ LogicalResult ModuleImport::convertIFunc(llvm::GlobalIFunc *ifunc) {
   return success();
 }
 
+/// Converts LLVM string, integer, and enum attributes into MLIR attributes,
+/// skipping those in `attributesToSkip` and emitting a warning at `loc` for
+/// any other unsupported attributes.
+static ArrayAttr
+processPassthroughAttrs(Location loc, MLIRContext *context,
+                        llvm::AttributeSet attributes,
+                        ArrayRef<StringLiteral> attributesToSkip = {}) {
+  SmallVector<Attribute> passthroughs;
+  for (llvm::Attribute attr : attributes) {
+    StringRef attrName;
+    if (attr.isStringAttribute())
+      attrName = attr.getKindAsString();
+    else
+      attrName = llvm::Attribute::getNameFromAttrKind(attr.getKindAsEnum());
+    if (llvm::is_contained(attributesToSkip, attrName))
+      continue;
+
+    auto keyAttr = StringAttr::get(context, attrName);
+    if (attr.isStringAttribute()) {
+      StringRef val = attr.getValueAsString();
+      if (val.empty()) {
+        // For string attributes without values, add only the attribute name.
+        passthroughs.push_back(keyAttr);
+        continue;
+      }
+      // For string attributes with a value, create a [name, value] pair.
+      passthroughs.push_back(
+          ArrayAttr::get(context, {keyAttr, StringAttr::get(context, val)}));
+      continue;
+    }
+    if (attr.isIntAttribute()) {
+      // For integer attributes, convert the value to a string and create a
+      // [name, value] pair.
+      auto val = std::to_string(attr.getValueAsInt());
+      passthroughs.push_back(
+          ArrayAttr::get(context, {keyAttr, StringAttr::get(context, val)}));
+      continue;
+    }
+    if (attr.isEnumAttribute()) {
+      // For enum attributes, add only the attribute name.
+      passthroughs.push_back(keyAttr);
+      continue;
+    }
+
+    emitWarning(loc)
+        << "'" << attrName
+        << "' attribute is invalid on current operation, skipping it";
+  }
+  return ArrayAttr::get(context, passthroughs);
+}
+
+/// Converts LLVM attributes from `globalVar` into MLIR attributes and adds them
+/// to `globalOp` as passthrough attributes.
+static void processPassthroughAttrs(llvm::GlobalVariable *globalVar,
+                                    GlobalOp globalOp) {
+  ArrayAttr passthroughAttr = processPassthroughAttrs(
+      globalOp.getLoc(), globalOp.getContext(), globalVar->getAttributes());
+  if (!passthroughAttr.empty())
+    globalOp.setPassthroughAttr(passthroughAttr);
+}
+
 LogicalResult ModuleImport::convertGlobal(llvm::GlobalVariable *globalVar) {
   // Insert the global after the last one or at the start of the module.
   OpBuilder::InsertionGuard guard = setGlobalInsertionPoint();
@@ -1460,6 +1521,8 @@ LogicalResult ModuleImport::convertGlobal(llvm::GlobalVariable *globalVar) {
   if (globalVar->hasComdat())
     globalOp.setComdatAttr(comdatMapping.lookup(globalVar->getComdat()));
 
+  processPassthroughAttrs(globalVar, globalOp);
+
   return success();
 }
 
@@ -2512,7 +2575,7 @@ static void processMemoryEffects(llvm::Function *func, LLVMFuncOp funcOp) {
 
 // List of LLVM IR attributes that map to an explicit attribute on the MLIR
 // LLVMFuncOp.
-static constexpr std::array kExplicitAttributes{
+static constexpr std::array kExplicitLLVMFuncOpAttributes{
     StringLiteral("aarch64_in_za"),
     StringLiteral("aarch64_inout_za"),
     StringLiteral("aarch64_new_za"),
@@ -2542,63 +2605,20 @@ static constexpr std::array kExplicitAttributes{
     StringLiteral("uwtable"),
     StringLiteral("vscale_range"),
     StringLiteral("willreturn"),
+    StringLiteral("memory"),
 };
 
+/// Converts LLVM attributes from `func` into MLIR attributes and adds them
+/// to `funcOp` as passthrough attributes, skipping those listed in
+/// `kExplicitLLVMFuncAttributes`.
 static void processPassthroughAttrs(llvm::Function *func, LLVMFuncOp funcOp) {
-  MLIRContext *context = funcOp.getContext();
-  SmallVector<Attribute> passthroughs;
   llvm::AttributeSet funcAttrs = func->getAttributes().getAttributes(
       llvm::AttributeList::AttrIndex::FunctionIndex);
-  for (llvm::Attribute attr : funcAttrs) {
-    // Skip the memory attribute since the LLVMFuncOp has an explicit memory
-    // attribute.
-    if (attr.hasAttribute(llvm::Attribute::Memory))
-      continue;
-
-    // Skip invalid type attributes.
-    if (attr.isTypeAttribute()) {
-      emitWarning(funcOp.getLoc(),
-                  "type attributes on a function are invalid, skipping it");
-      continue;
-    }
-
-    StringRef attrName;
-    if (attr.isStringAttribute())
-      attrName = attr.getKindAsString();
-    else
-      attrName = llvm::Attribute::getNameFromAttrKind(attr.getKindAsEnum());
-    auto keyAttr = StringAttr::get(context, attrName);
-
-    // Skip attributes that map to an explicit attribute on the LLVMFuncOp.
-    if (llvm::is_contained(kExplicitAttributes, attrName))
-      continue;
-
-    if (attr.isStringAttribute()) {
-      StringRef val = attr.getValueAsString();
-      if (val.empty()) {
-        passthroughs.push_back(keyAttr);
-        continue;
-      }
-      passthroughs.push_back(
-          ArrayAttr::get(context, {keyAttr, StringAttr::get(context, val)}));
-      continue;
-    }
-    if (attr.isIntAttribute()) {
-      auto val = std::to_string(attr.getValueAsInt());
-      passthroughs.push_back(
-          ArrayAttr::get(context, {keyAttr, StringAttr::get(context, val)}));
-      continue;
-    }
-    if (attr.isEnumAttribute()) {
-      passthroughs.push_back(keyAttr);
-      continue;
-    }
-
-    llvm_unreachable("unexpected attribute kind");
-  }
-
-  if (!passthroughs.empty())
-    funcOp.setPassthroughAttr(ArrayAttr::get(context, passthroughs));
+  ArrayAttr passthroughAttr =
+      processPassthroughAttrs(funcOp.getLoc(), funcOp.getContext(), funcAttrs,
+                              kExplicitLLVMFuncOpAttributes);
+  if (!passthroughAttr.empty())
+    funcOp.setPassthroughAttr(passthroughAttr);
 }
 
 void ModuleImport::processFunctionAttributes(llvm::Function *func,
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index 2685b5c91426e..e277d85596413 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -1081,6 +1081,89 @@ static void addRuntimePreemptionSpecifier(bool dsoLocalRequested,
     gv->setDSOLocal(true);
 }
 
+/// Attempts to add an attribute identified by `key`, optionally with the given
+/// `value`, using `addAttribute` function. Reports errors at `loc` if any. If
+/// the attribute has a kind known to LLVM IR, create the attribute of this
+/// kind, otherwise keep it as a string attribute. Performs additional checks
+/// for attributes known to have or not have a value in order to avoid
+/// assertions inside LLVM upon construction. Expects `addAttribute` callable to
+/// forward its arguments to a method that adds an attribute to an object.
+template <typename AddAttributeFn>
+static LogicalResult checkedAddAttribute(Location loc, llvm::LLVMContext &ctx,
+                                         AddAttributeFn &&addAttribute,
+                                         StringRef key,
+                                         StringRef value = StringRef()) {
+  auto kind = llvm::Attribute::getAttrKindFromName(key);
+  if (kind == llvm::Attribute::None) {
+    addAttribute(key, value);
+    return success();
+  }
+
+  if (llvm::Attribute::isIntAttrKind(kind)) {
+    if (value.empty())
+      return emitError(loc) << "LLVM attribute '" << key << "' expects a value";
+
+    int64_t result;
+    if (!value.getAsInteger(/*Radix=*/0, result))
+      addAttribute(llvm::Attribute::get(ctx, kind, result));
+    else
+      addAttribute(key, value);
+    return success();
+  }
+
+  if (!value.empty())
+    return emitError(loc) << "LLVM attribute '" << key
+                          << "' does not expect a value, found '" << value
+                          << "'";
+
+  addAttribute(kind);
+  return success();
+}
+
+/// Adds the attributes listed in the given array attribute using `addAttribute`
+/// callable. Reports error to `loc` if any and returns immediately. Expects
+/// `attributes` to be an array attribute containing either string attributes,
+/// treated as value-less LLVM attributes, or array attributes containing two
+/// string attributes, with the first string being the name of the corresponding
+/// LLVM attribute and the second string beings its value. Note that even
+/// integer attributes are expected to have their values expressed as strings.
+/// Expects `addAttribute` callable to forward its arguments to a method that
+/// adds an attribute to an object.
+template <typename AddAttributeFn>
+static LogicalResult
+forwardPassthroughAttributes(Location loc, llvm::LLVMContext &ctx,
+                             std::optional<ArrayAttr> attributes,
+                             AddAttributeFn &&addAttribute) {
+  if (!attributes)
+    return success();
+
+  for (Attribute attr : *attributes) {
+    if (auto stringAttr = dyn_cast<StringAttr>(attr)) {
+      if (failed(checkedAddAttribute(loc, ctx, addAttribute,
+                                     stringAttr.getValue())))
+        return failure();
+      continue;
+    }
+
+    auto arrayAttr = dyn_cast<ArrayAttr>(attr);
+    if (!arrayAttr || arrayAttr.size() != 2)
+      return emitError(loc) << "expected 'passthrough' to contain string "
+                               "or array attributes";
+
+    auto keyAttr = dyn_cast<StringAttr>(arrayAttr[0]);
+    auto valueAttr = dyn_cast<StringAttr>(arrayAttr[1]);
+    if (!keyAttr || !valueAttr)
+      return emitError(loc) << "expected arrays within 'passthrough' to "
+                               "contain two strings";
+
+    if (failed(checkedAddAttribute(loc, ctx, addAttribute, keyAttr.getValue(),
+                                   valueAttr.getValue())))
+      return failure();
+  }
+
+  return success();
+}
+
 LogicalResult ModuleTranslation::convertGlobalsAndAliases() {
   // Mapping from compile unit to its respective set of global variables.
   DenseMap<llvm::DICompileUnit *, SmallVector<llvm::Metadata *>> allGVars;
@@ -1191,6 +1274,14 @@ LogicalResult ModuleTranslation::convertGlobalsAndAliases() {
         }
       }
     }
+
+    // Forward the pass-through attributes to LLVM.
+    auto addAttribute = [var](auto &&...args) {
+      var->addAttribute(std::forward<decltype(args)>(args)...);
+    };
+    if (failed(forwardPassthroughAttributes(op.getLoc(), var->getContext(),
+                                            op.getPassthrough(), addAttribute)))
+      return failure();
   }
 
   // Create all llvm::GlobalAlias
@@ -1381,44 +1472,6 @@ LogicalResult ModuleTranslation::convertGlobalsAndAliases() {
   return success();
 }
 
-/// Attempts to add an attribute identified by `key`, optionally with the given
-/// `value` to LLVM function `llvmFunc`. Reports errors at `loc` if any. If the
-/// attribute has a kind known to LLVM IR, create the attribute of this kind,
-/// otherwise keep it as a string attribute. Performs additional checks for
-/// attributes known to have or not have a value in order to avoid assertions
-/// inside LLVM upon construction.
-static LogicalResult checkedAddLLVMFnAttribute(Location loc,
-                                               llvm::Function *llvmFunc,
-                                               StringRef key,
-                                               StringRef value = StringRef()) {
-  auto kind = llvm::Attribute::getAttrKindFromName(key);
-  if (kind == llvm::Attribute::None) {
-    llvmFunc->addFnAttr(key, value);
-    return success();
-  }
-
-  if (llvm::Attribute::isIntAttrKind(kind)) {
-    if (value.empty())
-      return emitError(loc) << "LLVM attribute '" << key << "' expects a value";
-
-    int64_t result;
-    if (!value.getAsInteger(/*Radix=*/0, result))
-      llvmFunc->addFnAttr(
-          llvm::Attribute::get(llvmFunc->getContext(), kind, result));
-    else
-      llvmFunc->addFnAttr(key, value);
-    return success();
-  }
-
-  if (!value.empty())
-    return emitError(loc) << "LLVM attribute '" << key
-                          << "' does not expect a value, found '" << value
-                          << "'";
-
-  llvmFunc->addFnAttr(kind);
-  return success();
-}
-
 /// Return a representation of `value` as metadata.
 static llvm::Metadata *convertIntegerToMetadata(llvm::LLVMContext &context,
                                                 const llvm::APInt &value) {
@@ -1454,45 +1507,6 @@ static llvm::MDNode *convertIntegerArrayToMDNode(llvm::LLVMContext &context,
   return llvm::MDNode::get(context, mdValues);
 }
 
-/// Attaches the attributes listed in the given array attribute to `llvmFunc`.
-/// Reports error to `loc` if any and returns immediately. Expects `attributes`
-/// to be an array attribute containing either string attributes, treated as
-/// value-less LLVM attributes, or array attributes containing two string
-/// attributes, with the first string being the name of the corresponding LLVM
-/// attribute and the second string beings its value. Note that even integer
-/// attributes are expected to have their values expressed as strings.
-static LogicalResult
-forwardPassthroughAttributes(Location loc, std::optional<ArrayAttr> attributes,
-                             llvm::Function *llvmFunc) {
-  if (!attributes)
-    return success();
-
-  for (Attribute attr : *attributes) {
-    if (auto stringAttr = dyn_cast<StringAttr>(attr)) {
-      if (failed(
-              checkedAddLLVMFnAttribute(loc, llvmFunc, stringAttr.getValue())))
-        return failure();
-      continue;
-    }
-
-    auto arrayAttr = dyn_cast<ArrayAttr>(attr);
-    if (!arrayAttr || arrayAttr.size() != 2)
-      return emitError(loc)
-             << "expected 'passthrough' to contain string or array attributes";
-
-    auto keyAttr = dyn_cast<StringAttr>(arrayAttr[0]);
-    auto valueAttr = dyn_cast<StringAttr>(arrayAttr[1]);
-    if (!keyAttr || !valueAttr)
-      return emitError(loc)
-             << "expected arrays within 'passthrough' to contain two strings";
-
-    if (failed(checkedAddLLVMFnAttribute(loc, llvmFunc, keyAttr.getValue(),
-                                         valueAttr.getValue())))
-      return failure();
-  }
-  return success();
-}
-
 LogicalResult ModuleTranslation::convertOneFunction(LLVMFuncOp func) {
   // Clear the block, branch value mappings, they are only relevant within one
   // function.
@@ -1864,8 +1878,12 @@ LogicalResult ModuleTranslation::convertFunctionSignatures() {
     }
 
     // Forward the pass-through attributes to LLVM.
+    auto addAttribute = [llvmFunc](auto &&...args) {
+      llvmFunc->addFnAttr(std::forward<decltype(args)>(args)...);
+    };
     if (failed(forwardPassthroughAttributes(
-            function.getLoc(), function.getPassthrough(), llvmFunc)))
+            function.getLoc(), llvmFunc->getContext(),
+            function.getPassthrough(), addAttribute)))
       return failure();
 
     // Convert visibility attribute.
diff --git a/mlir/test/Target/LLVMIR/Import/function-attributes.ll b/mlir/test/Target/LLVMIR/Import/function-attributes.ll
index 0b13645853cea..683a0c48d7377 100644
--- a/mlir/test/Target/LLVMIR/Import/function-attributes.ll
+++ b/mlir/test/Target/LLVMIR/Import/function-attributes.ll
@@ -426,3 +426,8 @@ declare void @nounwind_attribute() nounwind
 ; CHECK-LABEL: @willreturn_attribute
 ; CHECK-SAME: attributes {will_return}
 declare void @willreturn_attribute() willreturn
+
+// -----
+
+; expected-warning @unknown {{'preallocated' attribute is invalid on current operation, skipping it}}
+declare void @test() preallocated(i32)
diff --git a/mlir/test/Target/LLVMIR/Import/global-variables.ll b/mlir/test/Target/LLVMIR/Import/global-variables.ll
index b8bbdbab2e2ca..df75b1faa19be 100644
--- a/mlir/test/Target/LLVMIR/Import/global-variables.ll
+++ b/mlir/test/Target/LLVMIR/Import/global-variables.ll
@@ -186,19 +186,16 @@
 ; CHECK-SAME:  {addr_space = 0 : i32, dso_local} : !llvm.array<2 x f32>
 @array_constant = internal constant [2 x float] [float 1., float 2.]
 
-; CHECK: llvm.mlir.global internal constant @nested_array_constant
-; CHECK-SAME-LITERAL:  (dense<[[1, 2], [3, 4]]> : tensor<2x2xi32>)
-; CHECK-SAME-LITERAL:  {addr_space = 0 : i32, dso_local} : !llvm.array<2 x array<2 x i32>>
+; CHECK{LITERAL}: llvm.mlir.global internal constant @nested_array_constant(dense<[[1, 2], [3, 4]]> : tensor<2x2xi32>)
+; CHECK-SAME:  {addr_space = 0 : i32, dso_local} : !llvm.array<2 x array<2 x i32>>
 @nested_array_constant = internal constant [2 x [2 x i32]] [[2 x i32] [i32 1, i32 2], [2 x i32] [i32 3, i32 4]]
 
-; CHECK: llvm.mlir.global internal constant @nested_array_constant3
-; CHECK-SAME-LITERAL:  (dense<[[[1, 2], [3, 4]]]> : tensor<1x2x2xi32>)
-; CHECK-SAME-LITERAL:  {addr_space = 0 : i32, dso_local} : !llvm.array<1 x array<2 x array<2 x i32>>>
+; CHECK{LITERAL}: llvm.mlir.global internal constant @nested_array_constant3(dense<[[[1, 2], [3, 4]]]> : tensor<1x2x2xi32>)
+; CHECK-SAME:  {addr_space = 0 : i32, dso_local} : !llvm.array<1 x array<2 x array<2 x i32>>>
 @nested_array_constant3 = internal constant [1 x [2 x [2 x i32]]] [[2 x [2 x i32]] [[2 x i32] ...
[truncated]

Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

/// adds an attribute to an object.
template <typename AddAttributeFn>
static LogicalResult
forwardPassthroughAttributes(Location loc, llvm::LLVMContext &ctx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to return FailureOr<AttributeSet> similar to how we do it in the import? That way there would be no need to pass in the add attribute callback and possibly the LLVM interface changes are also not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you for the suggestion! I still ended up making some changes to the LLVM interface to allow adding attributes without converting from llvm::AttrBuilder to llvm::AttributeSet and back multiple times.

@@ -1313,7 +1313,8 @@ def LLVM_GlobalOp : LLVM_Op<"mlir.global",
OptionalAttr<StrAttr>:$section,
OptionalAttr<SymbolRefAttr>:$comdat,
OptionalAttr<DIGlobalVariableExpressionArrayAttr>:$dbg_exprs,
DefaultValuedAttr<Visibility, "mlir::LLVM::Visibility::Default">:$visibility_
DefaultValuedAttr<Visibility, "mlir::LLVM::Visibility::Default">:$visibility_,
OptionalAttr<ArrayAttr>:$passthrough
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc should be updated I believe.


// CHECK: @passthrough_combined = global i32 2, section "mysection", align 4 #[[ATTRS:[0-9]+]]
// CHECK: attributes #[[ATTRS]] = { norecurse "bss-section"="my_bss.1" }
llvm.mlir.global external @passthrough_combined(2 : i32) {alignment = 4 : i64, passthrough = ["norecurse", ["bss-section", "my_bss.1"]], section = "mysection"} : i32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly concerned that we use a "passthrough" blob to model important things like "section" which I would think should be inherent attribute of the global?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the "section" attribute is not part of the passthrough. I moved it before the passthrough attribute to avoid confusion. Or are you referring to the "bss-section"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I was referring to the section, I misparsed it.

That said I think my comment is still somehow applicable, when reading LLVM LangRef I thought the attributes would be either the ones listed in LangRef or some "target-specific" ones (for function), but for Global LangRef isn't clear and only lists 4 attributes.

What's is the expectation on what can be an attribute on a Global?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our intention is to import target-specific attributes on globals.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why don't we call this "target-specific-attrs"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"target-specific-attrs" might indeed be a more accurate name. I renamed it, thank you.

@VadimCurca VadimCurca force-pushed the vadimc/add_globalOp_passthrough branch 2 times, most recently from e3d1135 to 9a77444 Compare August 22, 2025 13:52
@VadimCurca VadimCurca requested review from joker-eph and gysit August 22, 2025 13:53
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes.

LGTM from my end.

Please wait with landing until the other reviewers had a chance to take a second look.

Copy link
Contributor

@Dinistro Dinistro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@VadimCurca VadimCurca force-pushed the vadimc/add_globalOp_passthrough branch from 9a77444 to 0b34ba0 Compare August 28, 2025 11:24
Adds a `target_specific_attrs` optional array attribute to
`mlir.global`, as well as conversions to and from LLVM attributes on
`llvm::GlobalVariable` objects. This is necessary to preserve unknown
attributes on global variables when converting to and from the LLVM
Dialect. Previously, any attributes on an `llvm::GlobalVariable` not
explicitly modeled by `mlir.global` were dropped during conversion.
@VadimCurca VadimCurca force-pushed the vadimc/add_globalOp_passthrough branch from 0b34ba0 to 2e16763 Compare August 28, 2025 11:29
@VadimCurca VadimCurca changed the title [MLIR] Add passthrough attribute to mlir.global [MLIR] Add target_specific_attrs attribute to mlir.global Aug 28, 2025
@VadimCurca VadimCurca requested a review from gysit August 28, 2025 11:31
Copy link
Contributor

@gysit gysit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

LGTM including the latest change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants