Reimplementing target description concept using DLTI attribute #92138
+964
−215
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
backend:AMDGPU
mlir:core
|
@llvm/pr-subscribers-mlir-dlti @llvm/pr-subscribers-backend-amdgpu Author: Niranjan Hasabnis (nhasabni) Changesand Interfaces. This is a newer implementation of PR #85141 and RFC by considering reviews and comments on the original PR. As an example of attributes supported by this commit: Patch is 62.15 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/92138.diff 14 Files Affected:
diff --git a/mlir/include/mlir/Dialect/DLTI/DLTI.h b/mlir/include/mlir/Dialect/DLTI/DLTI.h
index 5ac7c11e6ffee..f78e8bdc5eb98 100644
--- a/mlir/include/mlir/Dialect/DLTI/DLTI.h
+++ b/mlir/include/mlir/Dialect/DLTI/DLTI.h
@@ -21,6 +21,8 @@ namespace mlir {
namespace impl {
class DataLayoutEntryStorage;
class DataLayoutSpecStorage;
+class TargetSystemDescSpecAttrStorage;
+class TargetDeviceDescSpecAttrStorage;
} // namespace impl
//===----------------------------------------------------------------------===//
@@ -124,6 +126,150 @@ class DataLayoutSpecAttr
static constexpr StringLiteral name = "builtin.data_layout_spec";
};
+//===----------------------------------------------------------------------===//
+// TargetSystemDescSpecAttr
+//===----------------------------------------------------------------------===//
+
+/// A system description attribute is a list of device descriptors, each
+/// having a unique device ID
+class TargetSystemDescSpecAttr
+ : public Attribute::AttrBase<TargetSystemDescSpecAttr, Attribute,
+ impl::TargetSystemDescSpecAttrStorage,
+ TargetSystemDescSpecInterface::Trait> {
+public:
+ using Base::Base;
+
+ /// The keyword used for this attribute in custom syntax.
+ constexpr const static StringLiteral kAttrKeyword = "tsd_spec";
+
+ /// Returns a system descriptor attribute from the given system descriptor
+ static TargetSystemDescSpecAttr
+ get(MLIRContext *context, ArrayRef<TargetDeviceDescSpecInterface> entries);
+
+ /// Returns the list of entries.
+ TargetDeviceDescSpecListRef getEntries() const;
+
+ /// Return the device descriptor that matches the given device ID
+ TargetDeviceDescSpecInterface getDeviceDescForDeviceID(uint32_t deviceID);
+
+ /// Returns the specification containing the given list of keys. If the list
+ /// contains duplicate keys or is otherwise invalid, reports errors using the
+ /// given callback and returns null.
+ static TargetSystemDescSpecAttr
+ getChecked(function_ref<InFlightDiagnostic()> emitError, MLIRContext *context,
+ ArrayRef<TargetDeviceDescSpecInterface> entries);
+
+ /// Checks that the given list of entries does not contain duplicate keys.
+ static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
+ ArrayRef<TargetDeviceDescSpecInterface> entries);
+
+ /// Parses an instance of this attribute.
+ static TargetSystemDescSpecAttr parse(AsmParser &parser);
+
+ /// Prints this attribute.
+ void print(AsmPrinter &os) const;
+
+ static constexpr StringLiteral name = "builtin.target_system_description";
+};
+
+//===----------------------------------------------------------------------===//
+// TargetDeviceDescSpecAttr
+//===----------------------------------------------------------------------===//
+
+class TargetDeviceDescSpecAttr
+ : public Attribute::AttrBase<TargetDeviceDescSpecAttr, Attribute,
+ impl::TargetDeviceDescSpecAttrStorage,
+ TargetDeviceDescSpecInterface::Trait> {
+public:
+ using Base::Base;
+
+ /// The keyword used for this attribute in custom syntax.
+ constexpr const static StringLiteral kAttrKeyword = "tdd_spec";
+
+ /// Returns a system descriptor attribute from the given system descriptor
+ static TargetDeviceDescSpecAttr
+ get(MLIRContext *context, ArrayRef<DataLayoutEntryInterface> entries);
+
+ /// Returns the specification containing the given list of keys. If the list
+ /// contains duplicate keys or is otherwise invalid, reports errors using the
+ /// given callback and returns null.
+ static TargetDeviceDescSpecAttr
+ getChecked(function_ref<InFlightDiagnostic()> emitError, MLIRContext *context,
+ ArrayRef<DataLayoutEntryInterface> entries);
+
+ /// Checks that the given list of entries does not contain duplicate keys.
+ static LogicalResult verify(function_ref<InFlightDiagnostic()> emitError,
+ ArrayRef<DataLayoutEntryInterface> entries);
+
+ /// Returns the list of entries.
+ DataLayoutEntryListRef getEntries() const;
+
+ /// Parses an instance of this attribute.
+ static TargetDeviceDescSpecAttr parse(AsmParser &parser);
+
+ /// Prints this attribute.
+ void print(AsmPrinter &os) const;
+
+ /// Returns the device ID identifier.
+ StringAttr getDeviceIDIdentifier(MLIRContext *context);
+
+ /// Returns the device type identifier.
+ StringAttr getDeviceTypeIdentifier(MLIRContext *context);
+
+ /// Returns max vector op width identifier.
+ StringAttr getMaxVectorOpWidthIdentifier(MLIRContext *context);
+
+ /// Returns canonicalizer max iterations identifier.
+ StringAttr getCanonicalizerMaxIterationsIdentifier(MLIRContext *context);
+
+ /// Returns canonicalizer max num rewrites identifier.
+ StringAttr getCanonicalizerMaxNumRewritesIdentifier(MLIRContext *context);
+
+ /// Returns L1 cache size identifier
+ StringAttr getL1CacheSizeInBytesIdentifier(MLIRContext *context);
+
+ /// Returns the interface spec for device ID
+ /// Since we verify that the spec contains device ID the function
+ /// will return a valid spec.
+ DataLayoutEntryInterface getSpecForDeviceID(MLIRContext *context);
+
+ /// Returns the interface spec for device type
+ /// Since we verify that the spec contains device type the function
+ /// will return a valid spec.
+ DataLayoutEntryInterface getSpecForDeviceType(MLIRContext *context);
+
+ /// Returns the interface spec for max vector op width
+ /// Since max vector op width is an optional property, this function will
+ /// return a valid spec if the property is defined, otherwise it
+ /// will return an empty spec.
+ DataLayoutEntryInterface getSpecForMaxVectorOpWidth(MLIRContext *context);
+
+ /// Returns the interface spec for L1 cache size
+ /// Since L1 cache size is an optional property, this function will
+ /// return a valid spec if the property is defined, otherwise it
+ /// will return an empty spec.
+ DataLayoutEntryInterface getSpecForL1CacheSizeInBytes(MLIRContext *context);
+
+ /// Returns the interface spec for canonicalizer max iterations.
+ /// Since this is an optional property, this function will
+ /// return a valid spec if the property is defined, otherwise it
+ /// will return an empty spec.
+ DataLayoutEntryInterface
+ getSpecForCanonicalizerMaxIterations(MLIRContext *context);
+
+ /// Returns the interface spec for canonicalizer max num rewrites.
+ /// Since this is an optional property, this function will
+ /// return a valid spec if the property is defined, otherwise it
+ /// will return an empty spec.
+ DataLayoutEntryInterface
+ getSpecForCanonicalizerMaxNumRewrites(MLIRContext *context);
+
+ /// Return the value of device ID
+ uint32_t getDeviceID(MLIRContext *context);
+
+ static constexpr StringLiteral name = "builtin.target_device_description";
+};
+
} // namespace mlir
#include "mlir/Dialect/DLTI/DLTIDialect.h.inc"
diff --git a/mlir/include/mlir/Dialect/DLTI/DLTIBase.td b/mlir/include/mlir/Dialect/DLTI/DLTIBase.td
index 3572a99fad874..c9a054b3c1e51 100644
--- a/mlir/include/mlir/Dialect/DLTI/DLTIBase.td
+++ b/mlir/include/mlir/Dialect/DLTI/DLTIBase.td
@@ -27,6 +27,13 @@ def DLTI_Dialect : Dialect {
constexpr const static ::llvm::StringLiteral
kDataLayoutAttrName = "dlti.dl_spec";
+ // Top level attribute name for target system description
+ constexpr const static ::llvm::StringLiteral
+ kTargetSystemDescAttrName = "dlti.tsd_spec";
+
+ constexpr const static ::llvm::StringLiteral
+ kTargetDeviceDescAttrName = "dlti.tdd_spec";
+
// Constants used in entries.
constexpr const static ::llvm::StringLiteral
kDataLayoutEndiannessKey = "dlti.endianness";
@@ -48,6 +55,25 @@ def DLTI_Dialect : Dialect {
constexpr const static ::llvm::StringLiteral
kDataLayoutStackAlignmentKey = "dlti.stack_alignment";
+
+ // Constants used in target description part of DLTI
+ constexpr const static ::llvm::StringLiteral
+ kTargetDeviceIDKey = "dlti.device_id";
+
+ constexpr const static ::llvm::StringLiteral
+ kTargetDeviceTypeKey = "dlti.device_type";
+
+ constexpr const static ::llvm::StringLiteral
+ kTargetDeviceMaxVectorOpWidthKey = "dlti.max_vector_op_width";
+
+ constexpr const static ::llvm::StringLiteral
+ kTargetDeviceCanonicalizerMaxIterationsKey = "dlti.canonicalizer_max_iterations";
+
+ constexpr const static ::llvm::StringLiteral
+ kTargetDeviceCanonicalizerMaxNumRewritesKey = "dlti.canonicalizer_max_num_rewrites";
+
+ constexpr const static ::llvm::StringLiteral
+ kTargetDeviceL1CacheSizeInBytesKey = "dlti.L1_cache_size_in_bytes";
}];
let useDefaultAttributePrinterParser = 1;
@@ -71,6 +97,24 @@ def DLTI_DataLayoutSpecAttr : DialectAttr<
let convertFromStorage = "$_self";
}
+def DLTI_TargetSystemDescSpecAttr : DialectAttr<
+ DLTI_Dialect,
+ CPred<"::llvm::isa<::mlir::TargetSystemDescSpecAttr>($_self)">,
+ "Target system description part of DLTI"> {
+ let storageType = "::mlir::TargetSystemDescSpecAttr";
+ let returnType = "::mlir::TargetSystemDescSpecAttr";
+ let convertFromStorage = "$_self";
+}
+
+def DLTI_TargetDeviceDescSpecAttr : DialectAttr<
+ DLTI_Dialect,
+ CPred<"::llvm::isa<::mlir::TargetDeviceDescSpecAttr>($_self)">,
+ "Target device description part of DLTI"> {
+ let storageType = "::mlir::TargetDeviceDescSpecAttr";
+ let returnType = "::mlir::TargetDeviceDescSpecAttr";
+ let convertFromStorage = "$_self";
+}
+
def HasDefaultDLTIDataLayout : NativeOpTrait<"HasDefaultDLTIDataLayout"> {
let cppNamespace = "::mlir";
}
diff --git a/mlir/include/mlir/Dialect/DLTI/Traits.h b/mlir/include/mlir/Dialect/DLTI/Traits.h
index 5d86195305a95..44083d54c4cad 100644
--- a/mlir/include/mlir/Dialect/DLTI/Traits.h
+++ b/mlir/include/mlir/Dialect/DLTI/Traits.h
@@ -18,6 +18,7 @@ class DataLayoutSpecAttr;
namespace impl {
LogicalResult verifyHasDefaultDLTIDataLayoutTrait(Operation *op);
DataLayoutSpecInterface getDataLayoutSpec(Operation *op);
+TargetSystemDescSpecInterface getTargetSystemDescSpec(Operation *op);
} // namespace impl
/// Trait to be used by operations willing to use the implementation of the
@@ -37,6 +38,12 @@ class HasDefaultDLTIDataLayout
DataLayoutSpecInterface getDataLayoutSpec() {
return impl::getDataLayoutSpec(this->getOperation());
}
+
+ /// Returns the target system description specification as provided by DLTI
+ /// dialect
+ TargetSystemDescSpecInterface getTargetSystemDescSpec() {
+ return impl::getTargetSystemDescSpec(this->getOperation());
+ }
};
} // namespace mlir
diff --git a/mlir/include/mlir/IR/BuiltinOps.td b/mlir/include/mlir/IR/BuiltinOps.td
index eda24615c71ea..bdb4ce3ddfe20 100644
--- a/mlir/include/mlir/IR/BuiltinOps.td
+++ b/mlir/include/mlir/IR/BuiltinOps.td
@@ -78,6 +78,7 @@ def ModuleOp : Builtin_Op<"module", [
//===------------------------------------------------------------------===//
DataLayoutSpecInterface getDataLayoutSpec();
+ TargetSystemDescSpecInterface getTargetSystemDescSpec();
//===------------------------------------------------------------------===//
// OpAsmOpInterface Methods
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
index 76bf33e89a716..1584a13247dff 100644
--- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
+++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
@@ -23,11 +23,17 @@
namespace mlir {
class DataLayout;
class DataLayoutEntryInterface;
+class TargetDeviceDescSpecInterface;
+class TargetSystemDescSpecInterface;
using DataLayoutEntryKey = llvm::PointerUnion<Type, StringAttr>;
// Using explicit SmallVector size because we cannot infer the size from the
// forward declaration, and we need the typedef in the actual declaration.
using DataLayoutEntryList = llvm::SmallVector<DataLayoutEntryInterface, 4>;
using DataLayoutEntryListRef = llvm::ArrayRef<DataLayoutEntryInterface>;
+// using TargetDeviceDescSpecList =
+// llvm::SmallVector<TargetDeviceDescSpecInterface, 4>;
+using TargetDeviceDescSpecListRef =
+ llvm::ArrayRef<TargetDeviceDescSpecInterface>;
class DataLayoutOpInterface;
class DataLayoutSpecInterface;
class ModuleOp;
@@ -84,6 +90,24 @@ Attribute getDefaultGlobalMemorySpace(DataLayoutEntryInterface entry);
/// DataLayoutInterface if specified, otherwise returns the default.
uint64_t getDefaultStackAlignment(DataLayoutEntryInterface entry);
+/// return max vector op width from the specified DataLayoutEntry. If the
+/// property is missing from the entry, then return std::nullopt.
+std::optional<uint32_t> getMaxVectorOpWidth(DataLayoutEntryInterface entry);
+
+/// return L1 cache size in bytes from the specified DataLayoutEntry. If the
+/// property is missing from the entry, then return std::nullopt.
+std::optional<uint32_t> getL1CacheSizeInBytes(DataLayoutEntryInterface entry);
+
+/// return canonicalizer max iterations from the specified DataLayoutEntry.
+/// If the property is missing from the entry, then return std::nullopt.
+std::optional<int64_t>
+getCanonicalizerMaxIterations(DataLayoutEntryInterface entry);
+
+/// returncanonicalizer max num rewrites from the specified DataLayoutEntry.
+/// If the property is missing from the entry, then return std::nullopt.
+std::optional<int64_t>
+getCanonicalizerMaxNumRewrites(DataLayoutEntryInterface entry);
+
/// Given a list of data layout entries, returns a new list containing the
/// entries with keys having the given type ID, i.e. belonging to the same type
/// class.
@@ -95,6 +119,11 @@ DataLayoutEntryList filterEntriesForType(DataLayoutEntryListRef entries,
DataLayoutEntryInterface
filterEntryForIdentifier(DataLayoutEntryListRef entries, StringAttr id);
+/// Given a list of target device entries, returns the entry that has the given
+/// identifier as key, if such an entry exists in the list.
+TargetDeviceDescSpecInterface
+filterEntryForIdentifier(TargetDeviceDescSpecListRef entries, StringAttr id);
+
/// Verifies that the operation implementing the data layout interface, or a
/// module operation, is valid. This calls the verifier of the spec attribute
/// and checks if the layout is compatible with specs attached to the enclosing
@@ -106,6 +135,12 @@ LogicalResult verifyDataLayoutOp(Operation *op);
/// and dialect interfaces for type and identifier keys respectively.
LogicalResult verifyDataLayoutSpec(DataLayoutSpecInterface spec, Location loc);
+/// Verifies that a target system desc spec is valid. This dispatches to
+/// individual entry verifiers, and then to the verifiers implemented by the
+/// relevant dialect interfaces for identifier keys.
+LogicalResult verifyTargetSystemDescSpec(TargetSystemDescSpecInterface spec,
+ Location loc);
+
/// Divides the known min value of the numerator by the denominator and rounds
/// the result up to the next integer. Preserves the scalable flag.
llvm::TypeSize divideCeil(llvm::TypeSize numerator, uint64_t denominator);
@@ -137,6 +172,13 @@ class DataLayoutDialectInterface
return success();
}
+ /// Checks whether the given data layout entry is valid and reports any errors
+ /// at the provided location. Derived classes should override this.
+ virtual LogicalResult verifyEntry(TargetDeviceDescSpecInterface entry,
+ Location loc) const {
+ return success();
+ }
+
/// Default implementation of entry combination that combines identical
/// entries and returns null otherwise.
static DataLayoutEntryInterface
@@ -214,10 +256,33 @@ class DataLayout {
/// unspecified.
uint64_t getStackAlignment() const;
+ /// Returns for max vector op width if the property is defined for the given
+ /// device ID, otherwise return std::nullopt.
+ std::optional<uint32_t>
+ getMaxVectorOpWidth(TargetDeviceDescSpecInterface::DeviceID) const;
+
+ /// Returns for L1 cache size if the property is defined for the given
+ /// device ID, otherwise return std::nullopt.
+ std::optional<uint32_t>
+ getL1CacheSizeInBytes(TargetDeviceDescSpecInterface::DeviceID) const;
+
+ /// Returns for canonicalizer max iterations if the property is defined for
+ /// the given device ID, otherwise return std::nullopt.
+ std::optional<int64_t> getCanonicalizerMaxIterations(
+ TargetDeviceDescSpecInterface::DeviceID) const;
+
+ /// Returns for canonicalizer max rewrites if the property is defined for
+ /// the given device ID, otherwise return std::nullopt.
+ std::optional<int64_t> getCanonicalizerMaxNumRewrites(
+ TargetDeviceDescSpecInterface::DeviceID) const;
+
private:
/// Combined layout spec at the given scope.
const DataLayoutSpecInterface originalLayout;
+ /// Combined target system desc spec at the given scope.
+ const TargetSystemDescSpecInterface originalTargetSystemDesc;
+
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
/// List of enclosing layout specs.
SmallVector<DataLayoutSpecInterface, 2> layoutStack;
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
index 9edc885b9c5a9..75e609dde8fcf 100644
--- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
+++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
@@ -194,6 +194,182 @@ def DataLayoutSpecInterface : AttrInterface<"DataLayoutSpecInterface"> {
}];
}
+def TargetDeviceDescSpecInterface : AttrInterface<"TargetDeviceDescSpecInterface"> {
+ let cppNamespace = "::mlir";
+
+ let description = [{
+ Attribute interface describing a target device description specification.
+
+ A target device description specification is a list of device properties (key)
+ and their values for a specific device. The device is identified using "device_id"
+ (as a key and ui32 value) and "device_type" key which must have a string value.
+ Both "device_id" and "device_type" are mandatory keys. As an example, L1 cache
+ size could be a device property, and its value would be a device specific size.
+
+ A target device description specification is attached to a module as a module level
+ attribute.
+ }];
+
+ let methods = [
+ InterfaceMethod<
+ /*description=*/"Returns the list of layout entries.",
+ /*retTy=*/"::mlir::DataLayoutEntryListRef",
+ /*methodName=*/"getEntries",
+ /*args=*/(ins)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the entry related to the given identifier, if "
+ "present.",
+ /*retTy=*/"::mlir::DataLayoutEntryInterface",
+ /*methodName=*/"getSpecForIdentifier",
+ /*args=*/(ins "::mlir::StringAttr":$identifier),
+ /*methodBody=*/"",
+ /*defaultImplementation=*/[{
+ return ::mlir::detail::filterEntryForIdentifier($_attr.getEntries(),
+ identifier);
+ }]
+ >,
+ InterfaceMethod<
+ /*description=*/"Checks that the entry is well-formed, reports errors "
+ "at the provided location.",
+ /*retTy=*/"::mlir::LogicalResult",
+ /*methodName=*/"verifyEntry",
+ /*args=*/(ins "::mlir::Location":$loc),
+ /*methodBody=*/"",
+ /*defaultImplementation=*/[{ return ::mlir::success(); }]
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the device ID identifier.",
+ /*retTy=*/"::mlir::StringAttr",
+ /*methodName=*/"getDeviceIDIdentifier",
+ /*args=*/(ins "::mlir::MLIRContext *":$context)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the device type identifier.",
+ /*retTy=*/"::mlir::StringAttr",
+ /*methodName=*/"getDeviceTypeIdentifier",
+ /*args=*/(ins "::mlir::MLIRContext *":$context)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns the L1 cache size identifier.",
+ /*retTy=*/"::mlir::StringAttr",
+ /*methodName=*/"getMaxVectorOpWidthIdentifier",
+ /*args=*/(ins "::mlir::MLIRContext *":$context)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns canonicalizer max iterations identifier.",
+ /*retTy=*/"::mlir::StringAttr",
+ /*methodName=*/"getCanonicalizerMaxIterationsIdentifier",
+ /*args=*/(ins "::mlir::MLIRContext *":$context)
+ >,
+ InterfaceMethod<
+ /*description=*/"Returns canonicalizer max num rew...
[truncated]
|
|
@rengolin @joker-eph Please take a look. This is a DLTI based implementation for representing a target description as attributes. I've also added L1 cache size property and have used this property to calculate block size info in BlockMatMul pass. |
rengolin
requested review from
banach-space,
stellaraccident,
ftynse,
joker-eph and
nicolasvasilache
|
This is a continuation of #91670 which was unintentionally closed. It addresses @joker-eph's point by adding target properties, not compiler pass options. It also has some usage on different places. |
rengolin
reviewed
May 14, 2024
ftynse
requested changes
May 14, 2024
There was a problem hiding this comment.
Thanks! This generally goes in the right direction, let's iterate on this.
One large comment: this needs standalone tests for the target description part. For the existing DLTI part, we have a test pass that looks at certain kinds of ops and/or attributes and "interprets" them as requests to the data layout object, the results of these requests are appended back to ops as more attributes that we can FileCheck on. Something similar should be possible here.
joker-eph
reviewed
May 14, 2024
FYI: a PR can be reopened after being closed (not merged) instead of opening a new one. |
I tried this but the PR told me the branch was clean and couldn't reopen, even thought it wasn't. I didn't want to fight Github on this. :( |
adam-smnk
reviewed
May 16, 2024
|
@joker-eph @ftynse Thanks for your comments. I've fixed most of them (incl ODS based implementation). Test cases is WIP. I will also separate out the integration of target properties into the passes into a separate PR. |
joker-eph
reviewed
May 17, 2024
|
@joker-eph @ftynse I've addressed all of your comments. Pls take a look. Thanks. |
nhasabni
force-pushed
the
system_desc_cost_model_attrbased
branch
from
9399620 to
5bc3886
Compare
|
Windows bot fails with system error, ignore: |
@rengolin Not sure if this is related to the PR. All the unit tests seem to pass. |
It is not, just ignore. The Windows builder is super slow and unreliable. |
|
@ftynse @joker-eph Just a gentle ping for the review. |
nhasabni
force-pushed
the
system_desc_cost_model_attrbased
branch
from
1fbacea to
dfaacc2
Compare
nhasabni
force-pushed
the
system_desc_cost_model_attrbased
branch
from
c5e7278 to
c046d42
Compare
|
@ftynse are all your questions addressed? I'm hoping this is a good enough starting point, not a final design. I'm quite happy to navigate the questions in following PRs, so that we can do in parallel, even if they significantly change the design later. We're not fixed on the current state, we just think it's a good seed for a shared infrastructure. |
ftynse
approved these changes
Jun 17, 2024
| size could be a device property, and its value would be a device specific size. | ||
|
|
||
| A target device description specification is attached to a module as a module level | ||
| attribute. |
There was a problem hiding this comment.
Since attributes are owned by context anyway, it doesn't cost us much to allow them at all levels. It would also avoid unchecked indirection (what if someone decides to reorder the list in system description, for example?)
| checkValid(); | ||
| DataLayoutEntryInterface entry; | ||
| if (originalTargetSystemDesc) { | ||
| if (auto device = |
There was a problem hiding this comment.
Nit: expand auto unless the type is obvious.
| } | ||
| } | ||
|
|
||
| for (const auto &kvp : device_desc_keys) { |
There was a problem hiding this comment.
Nit: (auto &&[first, second] : ... works now.
joker-eph
reviewed
Jun 17, 2024
joker-eph
reviewed
Jun 17, 2024
joker-eph
reviewed
Jun 17, 2024
joker-eph
reviewed
Jun 17, 2024
| // is empty (meaning the spec is missing), returns std::nullopt. | ||
| std::optional<int64_t> | ||
| mlir::detail::getMaxVectorOpWidth(DataLayoutEntryInterface entry) { | ||
| if (entry == DataLayoutEntryInterface()) |
There was a problem hiding this comment.
Suggested change
| if (entry == DataLayoutEntryInterface()) | |
| if (!entry) |
But is this check useful?
joker-eph
reviewed
Jun 17, 2024
joker-eph
reviewed
Jun 17, 2024
nhasabni
force-pushed
the
system_desc_cost_model_attrbased
branch
from
1c58e6f to
856426f
Compare
joker-eph
reviewed
Jun 18, 2024
1. Use ODS framework for all of DLTI attrs 2. Removing need of MLIRContext in APIs 3. Removing canonicalizer heuristics from this PR
Mainly as follows:
- removing custom attribute parser/printer
- renaming SystemDescSpec to SystemSpec and TargetDeviceDescSpec to
TargetDeviceSpec
- using DeviceID as a type insted of uint32_t
- grammatical errors
Representing TargetSystemSpec as a set of key-value pairs where key is the DeviceID (string) and the value is TargetDeviceSpec.
nhasabni
force-pushed
the
system_desc_cost_model_attrbased
branch
from
8a5eafa to
7964793
Compare
|
@nhasabni Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
No problem! Thanks for the approval @joker-eph @ftynse! |
10 tasks
|
FYI, here's our tracking of the following steps: |
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
…92138) and Interfaces. This is a newer implementation of PR llvm#85141 and [RFC](https://discourse.llvm.org/t/rfc-target-description-and-cost-model-in-mlir/76990) by considering reviews and comments on the original PR. As an example of attributes supported by this commit: ``` module attributes { dlti.target_system_spec = #dlti.target_device_spec< #dlti.dl_entry<"dlti.device_id", 0: ui32>, #dlti.dl_entry<"dlti.device_type", "CPU">, #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192 : ui32>>, #dlti.target_device_spec < #dlti.dl_entry<"dlti.device_id", 1: ui32>, #dlti.dl_entry<"dlti.device_type", "GPU">, #dlti.dl_entry<"dlti.max_vector_op_width", 64 : ui32>>, #dlti.target_device_spec < #dlti.dl_entry<"dlti.device_id", 2: ui32>, #dlti.dl_entry<"dlti.device_type", "XPU">>> } ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
and Interfaces. This is a newer implementation of PR #85141 and RFC by considering reviews and comments on the original PR.
As an example of attributes supported by this commit: