[MLIR][LLVMIR][DLTI] Add #llvm.target, #llvm.data_layout and TargetAttrInterface #145899
Conversation
rolfmorel
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| // LLVM_TargetAttr | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| def LLVM_TargetAttr : LLVM_Attr<"Target", "target", [LLVM_TargetAttrInterface]> { |
There was a problem hiding this comment.
@rengolin, I am not sure what we want to do for verification here. Do we want to insist on the specified triple, cpu, and target_features being valid together. And do we verify that through constructing the relevant TargetMachine?
How would that work for dealing with IR which is annotated with a #llvm.target specifying a target that your current llvm-project isn't built with. Is it right that verification fails in that case? (Maybe it is, but it feels wrong as now verification succeeding or failing becomes a function of whether you built MLIR with the right flags.)
There was a problem hiding this comment.
One problem we faced on our side earlier was that it's not that easy to know you're constructing an invalid TargetMachine (link). But validating all possible combinations is just not viable. For downstream targets, you'd have to overload some verification to match their alternatives.
Other tools don't verify much either (other than completely crash if the strings are invalid), so I guess we can start with that. The discussion about which combinations are valid (link) are generally higher in the pipeline (front-end). At the IR level, we can only treat them as "gospel" and crash if we really can't build a TargetMachine.
rolfmorel
force-pushed
the
llvm-target-dlti
branch
from
680cc11 to
91560a5
Compare
|
Couple of pain points in need of suggestions/feedback:
|
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-mlir-dlti @llvm/pr-subscribers-mlir Author: Rolf Morel (rolfmorel) ChangesPatch is 38.47 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/145899.diff 22 Files Affected:
diff --git a/flang/include/flang/Optimizer/Dialect/FIRType.h b/flang/include/flang/Optimizer/Dialect/FIRType.h
index ecab12de55d61..83077aef8d08d 100644
--- a/flang/include/flang/Optimizer/Dialect/FIRType.h
+++ b/flang/include/flang/Optimizer/Dialect/FIRType.h
@@ -13,6 +13,7 @@
#ifndef FORTRAN_OPTIMIZER_DIALECT_FIRTYPE_H
#define FORTRAN_OPTIMIZER_DIALECT_FIRTYPE_H
+#include "mlir/Dialect/LLVMIR/LLVMTypes.h"
#include "mlir/IR/BuiltinAttributes.h"
#include "mlir/IR/BuiltinTypes.h"
#include "mlir/Interfaces/DataLayoutInterfaces.h"
diff --git a/mlir/include/mlir/Dialect/LLVMIR/CMakeLists.txt b/mlir/include/mlir/Dialect/LLVMIR/CMakeLists.txt
index cfad07e57021f..f1385cdff62be 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/LLVMIR/CMakeLists.txt
@@ -7,12 +7,26 @@ mlir_tablegen(LLVMOpsDialect.h.inc -gen-dialect-decls)
mlir_tablegen(LLVMOpsDialect.cpp.inc -gen-dialect-defs)
mlir_tablegen(LLVMOpsEnums.h.inc -gen-enum-decls)
mlir_tablegen(LLVMOpsEnums.cpp.inc -gen-enum-defs)
-mlir_tablegen(LLVMOpsAttrDefs.h.inc -gen-attrdef-decls
- -attrdefs-dialect=llvm)
+#For LLVMOpsAttrDefs.h.inc, see below.
mlir_tablegen(LLVMOpsAttrDefs.cpp.inc -gen-attrdef-defs
-attrdefs-dialect=llvm)
add_public_tablegen_target(MLIRLLVMOpsIncGen)
+# NB: Separate out LLVMOpsAttrDefs.h.inc generation as generating it
+# through LLVMOps.td ends up defining LLVMTargetFeaturesAttr even
+# though LLVMTargetFeaturesAttrDefs.* is responsible for that.
+set(LLVM_TARGET_DEFINITIONS LLVMAttrAndEnumDefs.td)
+mlir_tablegen(LLVMOpsAttrDefs.h.inc -gen-attrdef-decls -attrdefs-dialect=llvm)
+add_public_tablegen_target(MLIRLLVMAttrsIncGen)
+
+# NB: LLVMTargetFeaturesAttr is split out into its own file
+# to break a recursive dependency: LLVMInterfaces depends
+# on it, and other LLVMAttrs depending on LLVMInterfaces.
+set(LLVM_TARGET_DEFINITIONS LLVMTargetFeaturesAttrDefs.td)
+mlir_tablegen(LLVMTargetFeaturesAttrDefs.h.inc -gen-attrdef-decls)
+mlir_tablegen(LLVMTargetFeaturesAttrDefs.cpp.inc -gen-attrdef-defs)
+add_public_tablegen_target(MLIRLLVMTargetFeaturesAttrsIncGen)
+
set(LLVM_TARGET_DEFINITIONS LLVMTypes.td)
mlir_tablegen(LLVMTypes.h.inc -gen-typedef-decls -typedefs-dialect=llvm)
mlir_tablegen(LLVMTypes.cpp.inc -gen-typedef-defs -typedefs-dialect=llvm)
diff --git a/mlir/lib/Target/LLVMIR/DataLayoutImporter.h b/mlir/include/mlir/Dialect/LLVMIR/DataLayoutImporter.h
similarity index 82%
rename from mlir/lib/Target/LLVMIR/DataLayoutImporter.h
rename to mlir/include/mlir/Dialect/LLVMIR/DataLayoutImporter.h
index 88ceaf1a74e62..0f036f1c43492 100644
--- a/mlir/lib/Target/LLVMIR/DataLayoutImporter.h
+++ b/mlir/include/mlir/Dialect/LLVMIR/DataLayoutImporter.h
@@ -11,13 +11,14 @@
//
//===----------------------------------------------------------------------===//
-#ifndef MLIR_LIB_TARGET_LLVMIR_DATALAYOUTIMPORTER_H_
-#define MLIR_LIB_TARGET_LLVMIR_DATALAYOUTIMPORTER_H_
+#ifndef MLIR_LLVMIR_DATALAYOUTIMPORTER_H_
+#define MLIR_LLVMIR_DATALAYOUTIMPORTER_H_
#include "mlir/Dialect/LLVMIR/LLVMTypes.h"
#include "mlir/IR/BuiltinAttributes.h"
#include "mlir/Interfaces/DataLayoutInterfaces.h"
#include "llvm/ADT/MapVector.h"
+#include "llvm/IR/DataLayout.h"
namespace llvm {
class StringRef;
@@ -38,6 +39,8 @@ namespace detail {
/// null if the bit width is not supported.
FloatType getFloatType(MLIRContext *context, unsigned width);
+} // namespace detail
+
/// Helper class that translates an LLVM data layout to an MLIR data layout
/// specification. Only integer, float, pointer, alloca memory space, stack
/// alignment, and endianness entries are translated. The class also returns all
@@ -49,7 +52,21 @@ class DataLayoutImporter {
DataLayoutImporter(MLIRContext *context,
const llvm::DataLayout &llvmDataLayout)
: context(context) {
- translateDataLayout(llvmDataLayout);
+ // Transform the data layout to its string representation and append the
+ // default data layout string specified in the language reference
+ // (https://llvm.org/docs/LangRef.html#data-layout). The translation then
+ // parses the string and ignores the default value if a specific kind occurs
+ // in both strings. Additionally, the following default values exist:
+ // - non-default address space pointer specifications default to the default
+ // address space pointer specification
+ // - the alloca address space defaults to the default address space.
+ layoutStr = llvmDataLayout.getStringRepresentation();
+ translateDataLayoutFromStr();
+ }
+
+ DataLayoutImporter(MLIRContext *context, StringRef dataLayoutStr)
+ : layoutStr(dataLayoutStr), context(context) {
+ translateDataLayoutFromStr();
}
/// Returns the MLIR data layout specification translated from the LLVM
@@ -66,7 +83,7 @@ class DataLayoutImporter {
private:
/// Translates the LLVM `dataLayout` to an MLIR data layout specification.
- void translateDataLayout(const llvm::DataLayout &llvmDataLayout);
+ void translateDataLayoutFromStr();
/// Tries to parse the letter only prefix that identifies the specification
/// and removes the consumed characters from the beginning of the string.
@@ -125,8 +142,7 @@ class DataLayoutImporter {
DataLayoutSpecInterface dataLayout;
};
-} // namespace detail
} // namespace LLVM
} // namespace mlir
-#endif // MLIR_LIB_TARGET_LLVMIR_DATALAYOUTIMPORTER_H_
+#endif // MLIR_LLVMIR_DATALAYOUTIMPORTER_H_
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrAndEnumDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrAndEnumDefs.td
new file mode 100644
index 0000000000000..e34375076ffd1
--- /dev/null
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrAndEnumDefs.td
@@ -0,0 +1,10 @@
+//===-- LLVMAttrDefs.td - Solely LLVM Attribute and Enum definitions ----*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+include "mlir/Dialect/LLVMIR/LLVMAttrDefs.td"
+include "mlir/Dialect/LLVMIR/LLVMEnums.td"
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
index 790d2e77ea874..e563441d0102b 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
@@ -13,13 +13,138 @@ include "mlir/Dialect/LLVMIR/LLVMDialect.td"
include "mlir/Dialect/LLVMIR/LLVMInterfaces.td"
include "mlir/IR/AttrTypeBase.td"
include "mlir/IR/CommonAttrConstraints.td"
+include "mlir/Interfaces/DataLayoutInterfaces.td"
-// All of the attributes will extend this class.
-class LLVM_Attr<string name, string attrMnemonic,
- list<Trait> traits = [],
- string baseCppClass = "::mlir::Attribute">
- : AttrDef<LLVM_Dialect, name, traits, baseCppClass> {
- let mnemonic = attrMnemonic;
+//===----------------------------------------------------------------------===//
+// LLVM_TargetAttr
+//===----------------------------------------------------------------------===//
+
+def LLVM_TargetAttr : LLVM_Attr<"Target", "target",
+ [LLVM_TargetAttrInterface]> {
+ let summary = "LLVM target info: triple, chip, features";
+ let description = [{
+ An attribute to hold LLVM target information, specifying LLVM's target
+ `triple` string, the target `chip` string (i.e. the `cpu` string), and
+ target `features` string as an attribute. The latter two are optional.
+
+ Has facilities to obtain the corresponding `llvm::TargetMachine` and
+ `llvm::DataLayout`, given the relevant LLVM backend is loaded.
+
+ ---
+
+ Responds to DLTI-queries on the keys:
+ * A query for `"triple"` returns the `StringAttr` for the `triple`.
+ * A query for `"chip"` returns the `StringAttr` for the `chip`/`cpu`, if provided.
+ * A query for `"features"` returns the `TargetFeaturesAttr`, if provided.
+ * Individual features can be queried for on this attribute.
+ }];
+ let parameters = (ins "StringAttr":$triple,
+ OptionalParameter<"StringAttr">:$chip,
+ OptionalParameter<"TargetFeaturesAttr">:$features);
+
+ let assemblyFormat = [{`<` `triple` `=` $triple
+ (`,` `chip` `=` $chip^)?
+ (`,` qualified($features)^)? `>`}];
+
+ let extraClassDeclaration = [{
+ std::optional<llvm::TargetMachine *> targetMachine = std::nullopt;
+
+ FailureOr<llvm::TargetMachine *> getTargetMachine();
+
+ std::optional<llvm::DataLayout> dataLayout = std::nullopt;
+ FailureOr<llvm::DataLayout> getDataLayout();
+ FailureOr<Attribute> query(DataLayoutEntryKey key);
+ }];
+}
+
+//===----------------------------------------------------------------------===//
+// LLVM_DataLayoutAttr
+//===----------------------------------------------------------------------===//
+
+def LLVM_DataLayoutAttr
+ : LLVM_Attr<"DataLayout", "data_layout", [DataLayoutSpecInterface]> {
+ let summary = "LLVM data layout string, exposed through DLTI";
+ let description = [{
+ An attribute to hold a LLVM data layout string.
+
+ The LLVM data layout string is parsed and mapped to the corresponding MLIR
+ data layout specification. The `#llvm.data_layout` attribute then serves as
+ a proxy, forwarding all DLTI queries to the underlying MLIR data layout
+ specification.
+ }];
+ let parameters = (ins "StringAttr":$data_layout_str,
+ OptionalParameter<"DataLayoutSpecInterface", "{}">:$data_layout_spec);
+ let builders = [
+ AttrBuilder<(ins "llvm::StringRef":$data_layout_str), [{
+ auto importer = LLVM::DataLayoutImporter($_ctxt, data_layout_str);
+ auto dataLayoutSpec = importer.getDataLayout();
+ return $_get($_ctxt, mlir::StringAttr::get($_ctxt, data_layout_str), dataLayoutSpec);
+ }]>
+ ];
+ let assemblyFormat = "`<` $data_layout_str `>`";
+ let extraClassDeclaration = [{
+ template <typename Ty>
+ DataLayoutEntryList getSpecForType() {
+ return getDataLayoutSpec().getSpecForType(TypeID::get<Ty>());
+ }
+
+ inline ::mlir::FailureOr<::mlir::Attribute>
+ queryHelper(::mlir::DataLayoutEntryKey key) const {
+ return getDataLayoutSpec().queryHelper(key);
+ }
+
+ void bucketEntriesByType(
+ ::llvm::MapVector<::mlir::TypeID, ::mlir::DataLayoutEntryList> &types,
+ ::llvm::MapVector<::mlir::StringAttr,
+ ::mlir::DataLayoutEntryInterface> &ids) {
+ getDataLayoutSpec().bucketEntriesByType(types, ids);
+ };
+
+ ::mlir::DataLayoutSpecInterface
+ combineWith(ArrayRef<::mlir::DataLayoutSpecInterface> specs) const {
+ return getDataLayoutSpec().combineWith(specs);
+ }
+ DataLayoutEntryListRef getEntries() const { return getDataLayoutSpec().getEntries(); }
+ LogicalResult verifySpec(Location loc) {
+ return getDataLayoutSpec().verifySpec(loc);
+ }
+ StringAttr getEndiannessIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getEndiannessIdentifier(context);
+ }
+ StringAttr getDefaultMemorySpaceIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getDefaultMemorySpaceIdentifier(context);
+ }
+ StringAttr getAllocaMemorySpaceIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getAllocaMemorySpaceIdentifier(context);
+ }
+ StringAttr getManglingModeIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getManglingModeIdentifier(context);
+ }
+ StringAttr getProgramMemorySpaceIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getProgramMemorySpaceIdentifier(context);
+ }
+ StringAttr getGlobalMemorySpaceIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getGlobalMemorySpaceIdentifier(context);
+ }
+ StringAttr getStackAlignmentIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getStackAlignmentIdentifier(context);
+ }
+ StringAttr getFunctionPointerAlignmentIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getFunctionPointerAlignmentIdentifier(context);
+ }
+ StringAttr getLegalIntWidthsIdentifier(MLIRContext *context) const {
+ return getDataLayoutSpec().getLegalIntWidthsIdentifier(context);
+ }
+ ::mlir::DataLayoutEntryList getSpecForType(TypeID type) const {
+ return getDataLayoutSpec().getSpecForType(type);
+ }
+ ::mlir::DataLayoutEntryInterface getSpecForIdentifier(StringAttr identifier) const {
+ return getDataLayoutSpec().getSpecForIdentifier(identifier);
+ }
+ FailureOr<Attribute> query(DataLayoutEntryKey key) const {
+ return getDataLayoutSpec().query(key);
+ }
+ }];
}
//===----------------------------------------------------------------------===//
@@ -1241,69 +1366,6 @@ def LLVM_VScaleRangeAttr : LLVM_Attr<"VScaleRange", "vscale_range"> {
let assemblyFormat = "`<` struct(params) `>`";
}
-//===----------------------------------------------------------------------===//
-// TargetFeaturesAttr
-//===----------------------------------------------------------------------===//
-
-def LLVM_TargetFeaturesAttr : LLVM_Attr<"TargetFeatures", "target_features">
-{
- let summary = "LLVM target features attribute";
-
- let description = [{
- Represents the LLVM target features as a list that can be checked within
- passes/rewrites.
-
- Example:
- ```mlir
- #llvm.target_features<["+sme", "+sve", "+sme-f64f64"]>
- ```
-
- Then within a pass or rewrite the features active at an op can be queried:
-
- ```c++
- auto targetFeatures = LLVM::TargetFeaturesAttr::featuresAt(op);
-
- if (!targetFeatures.contains("+sme-f64f64"))
- return failure();
- ```
- }];
-
- let parameters = (ins OptionalArrayRefParameter<"StringAttr">:$features);
-
- let builders = [
- TypeBuilder<(ins "::llvm::StringRef":$features)>,
- TypeBuilder<(ins "::llvm::ArrayRef<::llvm::StringRef>":$features)>
- ];
-
- let extraClassDeclaration = [{
- /// Checks if a feature is contained within the features list.
- /// Note: Using a StringAttr allows doing pointer-comparisons.
- bool contains(::mlir::StringAttr feature) const;
- bool contains(::llvm::StringRef feature) const;
-
- bool nullOrEmpty() const {
- // Checks if this attribute is null, or the features are empty.
- return !bool(*this) || getFeatures().empty();
- }
-
- /// Returns the list of features as an LLVM-compatible string.
- std::string getFeaturesString() const;
-
- /// Finds the target features on the parent FunctionOpInterface.
- /// Note: This assumes the attribute name matches the return value of
- /// `getAttributeName()`.
- static TargetFeaturesAttr featuresAt(Operation* op);
-
- /// Canonical name for this attribute within MLIR.
- static constexpr StringLiteral getAttributeName() {
- return StringLiteral("target_features");
- }
- }];
-
- let assemblyFormat = "`<` `[` (`]`) : ($features^ `]`)? `>`";
- let genVerifyDecl = 1;
-}
-
//===----------------------------------------------------------------------===//
// UndefAttr
//===----------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
index 3ede857733242..14645d5dee95f 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h
@@ -14,8 +14,10 @@
#ifndef MLIR_DIALECT_LLVMIR_LLVMATTRS_H_
#define MLIR_DIALECT_LLVMIR_LLVMATTRS_H_
-#include "mlir/Dialect/LLVMIR/LLVMTypes.h"
#include "mlir/IR/OpImplementation.h"
+#include "mlir/Interfaces/DataLayoutInterfaces.h"
+#include "llvm/MC/TargetRegistry.h"
+#include "llvm/Target/TargetMachine.h"
#include <optional>
#include "mlir/Dialect/LLVMIR/LLVMOpsEnums.h.inc"
@@ -89,11 +91,17 @@ class TBAANodeAttr : public Attribute {
// TODO: this shouldn't be needed after we unify the attribute generation, i.e.
// --gen-attr-* and --gen-attrdef-*.
using cconv::CConv;
-using tailcallkind::TailCallKind;
using linkage::Linkage;
+using tailcallkind::TailCallKind;
} // namespace LLVM
} // namespace mlir
+// First obtain TargetFeaturesAttr definitions as it is used both an LLVMIR
+// interface and that interface and this attribute are turn required by another
+// LLVMIR attribute.
+#define GET_ATTRDEF_CLASSES
+#include "mlir/Dialect/LLVMIR/LLVMTargetFeaturesAttrDefs.h.inc"
+
#include "mlir/Dialect/LLVMIR/LLVMAttrInterfaces.h.inc"
#define GET_ATTRDEF_CLASSES
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.td
index b5ea8fc5da500..e924be32da10f 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.td
@@ -10,6 +10,7 @@
#define LLVMIR_DIALECT
include "mlir/IR/DialectBase.td"
+include "mlir/IR/AttrTypeBase.td"
def LLVM_Dialect : Dialect {
let name = "llvm";
@@ -123,4 +124,11 @@ def LLVM_Dialect : Dialect {
}];
}
+class LLVM_Attr<string name, string attrMnemonic,
+ list<Trait> traits = [],
+ string baseCppClass = "::mlir::Attribute">
+ : AttrDef<LLVM_Dialect, name, traits, baseCppClass> {
+ let mnemonic = attrMnemonic;
+}
+
#endif // LLVMIR_DIALECT
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMInterfaces.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMInterfaces.td
index 138170f8c8762..0d2603debbc28 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMInterfaces.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMInterfaces.td
@@ -14,6 +14,7 @@
#define LLVMIR_INTERFACES
include "mlir/IR/OpBase.td"
+include "mlir/Interfaces/DataLayoutInterfaces.td"
def FastmathFlagsInterface : OpInterface<"FastmathFlagsInterface"> {
let description = [{
@@ -532,4 +533,56 @@ def LLVM_DIRecursiveTypeAttrInterface
];
}
+def LLVM_TargetAttrInterface
+ : AttrInterface<"TargetAttrInterface", [DLTIQueryInterface]> {
+ let description = [{
+ Interface for attributes that describe LLVM targets.
+
+ These attributes should be able to return the specified target
+ `triple`, `chip` and `features` and are expected to be able to
+ produce the corresponding `llvm::TargetMachine` and
+ `llvm::DataLayout`. These methods can fail in case the backend
+ is not available.
+
+ Implementing attributes should provide a
+ `DLTIQueryInterface::query()` implementation which responds to
+ keys `"triple"`, `"chip"` and `"features"` by returning an
+ appropriate `StringAttr`, `StringAttr` and
+ `LLVM_TargetFeaturesAttr`.
+ }];
+ let cppNamespace = "::mlir::LLVM";
+ let methods = [
+ InterfaceMethod<
+ /*description=*/"Returns the target triple identifier.",
+ /*retTy=*/"::llvm::StringRef",
+ /*methodName=*/"getTriple",
+ /*args=*/(ins)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the target chip (i.e. \"cpu\") identifier.",
+ /*retTy=*/"::llvm::StringRef",
+ /*methodName=*/"getChip",
+ /*args=*/(ins)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the target features as a string.",
+ /*retTy=*/"::mlir::LLVM::TargetFeaturesAttr",
+ /*methodName=*/"getFeatures",
+ /*args=*/(ins)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the target machine.",
+ /*retTy=*/"FailureOr<::llvm::TargetMachine *>",
+ /*methodName=*/"getTargetMachine",
+ /*args=*/(ins)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the data layout associated to the target machine.",
+ /*retTy=*/"FailureOr<::llvm::DataLayout>",
+ /*methodName=*/"getDataLayout",
+ /*args=*/(ins)
+ >
+ ];
+}
+
#endif // LLVMIR_INTERFACES
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index f4c1640098320..36a6291b5e3a8 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -14,6 +14,7 @@
#define LLVMIR_OPS
include "mlir/Dialect/LLVMIR/LLVMAttrDefs.td"
+include "mlir/Dialect/LLVMIR/LLVMTargetFeaturesAttrDefs.td"
include "mlir/Dialect/LLVMIR/LLVMEnums.td"
include...
[truncated]
|
|
@fabianmcg thanks for pointing that out! That should resolve bullet 4 up above I believe. Any other feedback on this PR would be much appreciated. In particular any help with addressing the remaining bullets:
|
I'd argue is preferable to create
I'd recommend using the struct directive, that way the elements can appear in any order, and should handle the optional elements correctly. https://mlir.llvm.org/docs/DefiningDialects/AttributesAndTypes/#struct-directive If I recall correctly, the above issue is caused because the parser generated by MLIR uses only one lookahead token, so the parser doesn't know whether it's parsing chip or features because they both start with a Also, I'm not sure whether making
I imagine you want this for caching the TM. But why do you want this? When is it going to be reused? I think we should use it only to import the DL for now.
|
Older Arm targets didn't need a chip because the micro-architecture was encoded in the target triple. There can be the case where passing a chip name is not just confusing but wrong (if the target doesn't recognize the chip but can build a target with the correct set of features). For x86 and GPUs the chip names carry a lot of information, that is not encoded in the triple, so become necessary. Arguably we don't usually care about armv7 or lower, but I'll let @banach-space comment on that. |
The DL generator is one of the users, not the only one. The cost model will be making a lot of enquires about the target over the life of the compiler passes, so this cache is necessary to avoid constructing it for every invocation. A storage class should do the trick, but may need some massaging at this level. |
In that particular case, I'd prefer an empty chip or a string indicating this is a special case. Because as you say, there are a lot of targets where the chip does carry a lot info, therefore I think we should handle the common case of required chip. |
Do you already have the design of how that looks? I'm not saying it's not necessary, I'm arguing that until such use is ready to land we shouldn't put it in, as we would be adding unused features, and it's easy to add later when the use is ready. |
Empty string would be fine, yes. If Arm is fine with making it compulsory (and not empty), we should. Validation would be so much easier. If not, then we may need some custom validation per target to make sure that for the ones where the info is in the chip, it must not be empty. |
I'd be ok with that, too. |
Issue 1. - features is now a StringAttr Issue 2. - use struct for attribute parsing Issue 3. - forgo trying to cache llvm::TargetMachine Issue 4. - keep code that needs to link to llvm libs in mlir/Target
|
@fabianmcg , I have addressed all that has been discussed up above. Could you have another look? |
|
|
||
| llvm::InitializeAllTargets(); | ||
| llvm::InitializeAllTargetMCs(); | ||
|
|
There was a problem hiding this comment.
We shouldn't initialize targets we are not going to use.
| llvm::InitializeAllTargets(); | |
| llvm::InitializeAllTargetMCs(); |
There was a problem hiding this comment.
My understanding is that this is like tools like mlir-opt registering all the dialects and their extensions:
llvm-project/llvm/include/llvm/Support/TargetSelect.h
Lines 58 to 89 in ef98e24
If you have a suggestion for how to "initialize" just the relevant backends, I am all ears.
There was a problem hiding this comment.
Ok, I'd argue that a better approach would be having a target attribute for each LLVM target, that way only used targets get initialized.
There was a problem hiding this comment.
LLVM's opt does the same thing. There are not many ways around it as we don't know at compile time which targets will be called at runtime. I'd say this is an exercise for later, once the project as a whole decides how to manage that in a more sensible way.
There was a problem hiding this comment.
We do have one: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Target/LLVM/NVVM/Target.cpp#L124-L129
If each target has it's own attribute then the problem is solved.
Another option, is to add a target name parameter to the attribute, and init only that target.
| @@ -11,8 +11,8 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #ifndef MLIR_LIB_TARGET_LLVMIR_DATALAYOUTIMPORTER_H_ | |||
| #define MLIR_LIB_TARGET_LLVMIR_DATALAYOUTIMPORTER_H_ | |||
| #ifndef MLIR_DIALECT_LLVMIR_DATALAYOUTIMPORTER_H | |||
There was a problem hiding this comment.
What is the need to move this file to the LLVM dialect?
There was a problem hiding this comment.
So we can have a #llvm.data_layout<DATA_LAYOUT_STR> attribute. The DATA_LAYOUT_STR is parsed by this importer and yields an unwieldly
#dlti.dl_spec<"dlti.endianness" = "little", "dlti.mangling_mode" = "e", "dlti.legal_int_widths" = array<i32: 8, 16, 32, 64>, "dlti.stack_alignment" = 128 : i64, !llvm.ptr<270> = dense<32> : vector<4xi64>, !llvm.ptr<271> = dense<32> : vector<4xi64>, !llvm.ptr<272> = dense<64> : vector<4xi64>, !llvm.ptr = dense<64> : vector<4xi64>, i64 = dense<64> : vector<2xi64>, i128 = dense<128> : vector<2xi64>, i1 = dense<8> : vector<2xi64>, i8 = dense<8> : vector<2xi64>, i16 = dense<16> : vector<2xi64>, i32 = dense<32> : vector<2xi64>, f80 = dense<128> : vector<2xi64>, f16 = dense<16> : vector<2xi64>, f64 = dense<64> : vector<2xi64>, f128 = dense<128> : vector<2xi64>>
(instead of #llvm.data_layout<"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128">).
What the attribute does is to just show the compact encoding, i.e. the string. It serves as a proxy to the dl_spec representation, which is kept hidden.
There was a problem hiding this comment.
Having these files in LLVM dialect is actually independent of whether we have #llvm.data_layout. Either way the pass that converts a #llvm.target attr to a representation of MLIR's data layout spec will need this parser. As this pass cannot depend on code in mlir/lib/Target, the importer should be in the LLVM dialect.
There was a problem hiding this comment.
In general it doesn't matter if the textual version of the IR looks ugly, it's IR. It's preferable to have only one way to specify things, and in this case I'd argue that the verbose approach is better.
Re the second comment. You can make the Attr interface return the DL attribute, that way the interface impl and the importer still live in target.
There was a problem hiding this comment.
Side note: why is the DLTI using dense attributes for storing integers?
The example above would be much shorter with simple integers I think.
| #include "llvm/IR/DataLayout.h" | ||
| #include "llvm/Target/TargetMachine.h" |
There was a problem hiding this comment.
Why do we need these?
| #include "llvm/IR/DataLayout.h" | |
| #include "llvm/Target/TargetMachine.h" |
There was a problem hiding this comment.
As the mlir::LLVM::TargetAttrInterface has methods that return the DataLayout and TargetMachine.
Look at mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h to see that having dependency on LLVM's headers is not a big deal. For clarity's sake, due moving the parts that require linking to mlir/Target the LLVM dialect does not gain any linking dependencies.
There was a problem hiding this comment.
Actually those dependencies there are spurious and legacy, see #150692 for removal
Ideally the LLVM dialect shouldn't depend on llvm/IR.
| @@ -6,14 +6,13 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "DataLayoutImporter.h" | |||
| #include "mlir/Dialect/LLVMIR/DataLayoutImporter.h" | |||
There was a problem hiding this comment.
Same thing, why move the file?
|
|
||
| bool passFailed = false; | ||
|
|
||
| mod->walk([&](ModuleOp mod) { |
There was a problem hiding this comment.
Why does it need to be a builtin::ModuleOp?
There was a problem hiding this comment.
Fair. Should it be Operation as that's the only common ancestor between builtin::ModuleOp and gpu::ModuleOp?
There was a problem hiding this comment.
I would use an Operation pass, and not perform any walk but just lookup the current operation and get a LLVMTargetAttr directly from there.
| // LLVM_DataLayoutAttr | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| def LLVM_DataLayoutAttr |
There was a problem hiding this comment.
Why do we need an LLVM dedicated attr? AFAIK we already were able to store data layouts in MLIR and translate them to LLVM IR, did I miss a change there?
There was a problem hiding this comment.
As above: due to ungainly DLTI attributes.
Compare with this attribute:
module attributes {dlti.dl_spec = #llvm.data_layout<"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128">, llvm.target = #llvm.target<triple = "x86_64-unknown-linux", chip = "skylake", features = "+mmx,+sse">} {
}to without:
module attributes {dlti.dl_spec = #dlti.dl_spec<"dlti.endianness" = "little", "dlti.mangling_mode" = "e", "dlti.legal_int_widths" = array<i32: 8, 16, 32, 64>, "dlti.stack_alignment" = 128 : i64, !llvm.ptr<270> = dense<32> : vector<4xi64>, !llvm.ptr<271> = dense<32> : vector<4xi64>, !llvm.ptr<272> = dense<64> : vector<4xi64>, !llvm.ptr = dense<64> : vector<4xi64>, i64 = dense<64> : vector<2xi64>, i128 = dense<128> : vector<2xi64>, i1 = dense<8> : vector<2xi64>, i8 = dense<8> : vector<2xi64>, i16 = dense<16> : vector<2xi64>, i32 = dense<32> : vector<2xi64>, f80 = dense<128> : vector<2xi64>, f16 = dense<16> : vector<2xi64>, f64 = dense<64> : vector<2xi64>, f128 = dense<128> : vector<2xi64>>, llvm.target = #llvm.target<triple = "x86_64-unknown-linux", chip = "skylake", features = "+mmx,+sse">} {
}There was a problem hiding this comment.
Yeah, as above, I would argue verbose is better, it doesn't have to look pretty.
| void LLVM::registerLLVMTargetInterfaceExternalModels( | ||
| DialectRegistry ®istry) { | ||
| registry.addExtension(+[](MLIRContext *ctx, LLVM::LLVMDialect *dialect) { | ||
| LLVM::TargetAttr::attachInterface<LLVMTargetAttrImpl>(*ctx); |
There was a problem hiding this comment.
There is an issue with the way you're trying to set this all up I believe.
- We always should have a "promised interface" for in-tree external interface (which you're correctly doing already)
- That means the interface here will already be linked and loaded whenever the LLVMTargetAttr will be used. Which forces to link the LLVM backend, which does not seem desirable.
But actually there is a straightforward solution here: just not use an interface at all!
So: can we step back and see what this interface is meant to provide and if we can get away without having it?
Do we anticipate other LLVMTargetAttr to provide this interface?
There was a problem hiding this comment.
I would
- remove the interface
- Make the pass operate directly on LLVMTargetAttr.
- Move the implementation of the interface as private functions in the pass (getting DataLayout and TargetMachine from a LLVMTargetAttr).
That will make the pass itself dependent on the LLVM backend, but nothing else.
There was a problem hiding this comment.
That means the interface here will already be linked and loaded whenever the LLVMTargetAttr will be used. Which forces to link the LLVM backend, which does not seem desirable.
If the interface is never used (ie. appears in a cast) then the promise never asserts and one doesn't have to register the interface, so in theory it only introduces an optional dependency, and a hard dep on users of the interface, or did the mechanism change?
Now, any impl of the data layout importer will need to load a backend, so that's a hard dependency on the mechanism. I would argue we should split some target information from the LLVM backend, but that's another conversation.
|
|
||
| mod->walk([&](ModuleOp mod) { | ||
| auto targetAttr = | ||
| mod->getAttrOfType<LLVM::TargetAttrInterface>("llvm.target"); |
There was a problem hiding this comment.
We shouldn't have a magic strings, can you define "llvm.target" as a let discardableAttrs = (ins ... on the dialect instead?
Adds the
#llvm.target<triple = $TRIPLE, chip = $CHIP, #llvm.target_features<$FEATURES>>attribute and#llvm.data_layout<DATA_LAYOUT_STR>attribute with a-llvm-data-layout-from-targetpass to derive the latter from the former (using the existingDataLayoutImporter). Both attributes implement the relevant DLTI-interfaces to expose thetriple,chip(AKAcpu) andfeatureson#llvm.targetand the fullDataLayoutSpecInterfaceon#llvm.data_layout.#llvm.data_layoutconverts into a (fully verbose)#dlti.dl_specin case it needs to combine with an already present#dlti.dl_spec, e.g. one which specifies the index size.Adds a
TargetAttrInterfacewhich can be implemented by all attributes representing LLVM targets.Similar to the Draft PR #78073.
RFC on which this PR is based: https://discourse.llvm.org/t/mandatory-data-layout-in-the-llvm-dialect/85875