-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[MLIR] Add target_specific_attrs attribute to mlir.global #154706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -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, |
There was a problem hiding this comment.
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?
aaa040d
to
4d325a0
Compare
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-llvm-ir Author: Vadim Curcă (VadimCurca) ChangesAdds a 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:
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]
|
There was a problem hiding this 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
mlir/test/Target/LLVMIR/llvmir.mlir
Outdated
|
||
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
e3d1135
to
9a77444
Compare
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
9a77444
to
0b34ba0
Compare
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.
0b34ba0
to
2e16763
Compare
There was a problem hiding this 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.
Adds a
target_specific_attrs
optional array attribute tomlir.global
, as well as conversions to and from LLVM attributes onllvm::GlobalVariable
objects. This is necessary to preserve unknown attributes on global variables when converting to and from the LLVM Dialect. Previously, any attributes on anllvm::GlobalVariable
not explicitly modeled bymlir.global
were dropped during conversion.