Skip to content

Commit c046d42

Browse files
nhasabninhasabni
committed
Addressing review comments
1 parent 4b30dc1 commit c046d42

File tree

6 files changed

+35
-47
lines changed

6 files changed

+35
-47
lines changed

‎mlir/include/mlir/Dialect/DLTI/DLTIAttrs.td

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -189,18 +189,6 @@ def DLTI_TargetDeviceSpecAttr :
189189

190190
/// Returns L1 cache size identifier
191191
StringAttr getL1CacheSizeInBytesIdentifier();
192-
193-
/// Returns the interface spec for max vector op width
194-
/// Since max vector op width is an optional property, this function will
195-
/// return a valid spec if the property is defined, otherwise it
196-
/// will return an empty spec.
197-
DataLayoutEntryInterface getSpecForMaxVectorOpWidth();
198-
199-
/// Returns the interface spec for L1 cache size
200-
/// Since L1 cache size is an optional property, this function will
201-
/// return a valid spec if the property is defined, otherwise it
202-
/// will return an empty spec.
203-
DataLayoutEntryInterface getSpecForL1CacheSizeInBytes();
204192
}];
205193
}
206194

‎mlir/include/mlir/Interfaces/DataLayoutInterfaces.td

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -239,19 +239,19 @@ def TargetDeviceSpecInterface : AttrInterface<"TargetDeviceSpecInterface"> {
239239
/*defaultImplementation=*/[{ return ::mlir::success(); }]
240240
>,
241241
InterfaceMethod<
242-
/*description=*/"Returns the entry related to the given identifier, if "
243-
"present. Otherwise, return empty spec.",
244-
/*retTy=*/"::mlir::DataLayoutEntryInterface",
245-
/*methodName=*/"getSpecForMaxVectorOpWidth",
246-
/*args=*/(ins)
242+
/*description=*/"Returns max vector op width identifier. ",
243+
/*retTy=*/"::mlir::StringAttr",
244+
/*methodName=*/"getMaxVectorOpWidthIdentifier",
245+
/*args=*/(ins),
246+
/*methodBody=*/""
247247
>,
248248
InterfaceMethod<
249-
/*description=*/"Returns the entry related to the given identifier, if "
250-
"present. Otherwise, return empty spec.",
251-
/*retTy=*/"::mlir::DataLayoutEntryInterface",
252-
/*methodName=*/"getSpecForL1CacheSizeInBytes",
253-
/*args=*/(ins)
254-
>
249+
/*description=*/"Returns L1 cache size identifier identifier. ",
250+
/*retTy=*/"::mlir::StringAttr",
251+
/*methodName=*/"getL1CacheSizeInBytesIdentifier",
252+
/*args=*/(ins),
253+
/*methodBody=*/""
254+
>,
255255
];
256256
}
257257

‎mlir/lib/Dialect/DLTI/DLTI.cpp

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ using namespace mlir;
2424
#include "mlir/Dialect/DLTI/DLTIDialect.cpp.inc"
2525

2626
#define GET_ATTRDEF_CLASSES
27-
#include <mlir/Dialect/DLTI/DLTIAttrs.cpp.inc>
27+
#include "mlir/Dialect/DLTI/DLTIAttrs.cpp.inc"
2828

2929
#define DEBUG_TYPE "dlti"
3030

@@ -301,6 +301,8 @@ void DataLayoutSpecAttr::print(AsmPrinter &os) const {
301301
//===----------------------------------------------------------------------===//
302302

303303
namespace mlir {
304+
/// A FieldParser for key-value pairs of DeviceID-target device spec pairs that
305+
/// make up a target system spec.
304306
template <>
305307
struct FieldParser<DeviceIDTargetDeviceSpecPair> {
306308
static FailureOr<DeviceIDTargetDeviceSpecPair> parse(AsmParser &parser) {
@@ -340,8 +342,9 @@ inline AsmPrinter &operator<<(AsmPrinter &printer,
340342
LogicalResult
341343
TargetDeviceSpecAttr::verify(function_ref<InFlightDiagnostic()> emitError,
342344
ArrayRef<DataLayoutEntryInterface> entries) {
343-
// Entries in tdd_spec can only have StringAttr as key. It does not support
344-
// type as a key. Hence not reusing DataLayoutEntryInterface::verify.
345+
// Entries in a target device spec can only have StringAttr as key. It does
346+
// not support type as a key. Hence not reusing
347+
// DataLayoutEntryInterface::verify.
345348
DenseSet<StringAttr> ids;
346349
for (DataLayoutEntryInterface entry : entries) {
347350
if (auto type = llvm::dyn_cast_if_present<Type>(entry.getKey())) {
@@ -389,14 +392,6 @@ StringAttr TargetDeviceSpecAttr::getL1CacheSizeInBytesIdentifier() {
389392
.getStringAttr(DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey);
390393
}
391394

392-
DataLayoutEntryInterface TargetDeviceSpecAttr::getSpecForMaxVectorOpWidth() {
393-
return getSpecForIdentifier(getMaxVectorOpWidthIdentifier());
394-
}
395-
396-
DataLayoutEntryInterface TargetDeviceSpecAttr::getSpecForL1CacheSizeInBytes() {
397-
return getSpecForIdentifier(getL1CacheSizeInBytesIdentifier());
398-
}
399-
400395
//===----------------------------------------------------------------------===//
401396
// TargetSystemSpecAttr
402397
//===----------------------------------------------------------------------===//
@@ -407,10 +402,11 @@ TargetSystemSpecAttr::verify(function_ref<InFlightDiagnostic()> emitError,
407402
DenseSet<TargetSystemSpecInterface::DeviceID> device_ids;
408403

409404
for (const auto &entry : entries) {
410-
TargetDeviceSpecInterface tdd_spec = entry.second;
405+
TargetDeviceSpecInterface target_device_spec = entry.second;
411406

412407
// First verify that a target device spec is valid.
413-
if (failed(TargetDeviceSpecAttr::verify(emitError, tdd_spec.getEntries())))
408+
if (failed(TargetDeviceSpecAttr::verify(emitError,
409+
target_device_spec.getEntries())))
414410
return failure();
415411

416412
// Check that device IDs are unique across all entries.
@@ -462,6 +458,7 @@ class TargetDataLayoutInterface : public DataLayoutDialectInterface {
462458
} // namespace
463459

464460
namespace {
461+
/// An interface to check entries of a target device spec.
465462
class SystemDescSpecInterface : public DataLayoutDialectInterface {
466463
public:
467464
using DataLayoutDialectInterface::DataLayoutDialectInterface;
@@ -485,7 +482,7 @@ class SystemDescSpecInterface : public DataLayoutDialectInterface {
485482
void DLTIDialect::initialize() {
486483
addAttributes<
487484
#define GET_ATTRDEF_LIST
488-
#include <mlir/Dialect/DLTI/DLTIAttrs.cpp.inc>
485+
#include "mlir/Dialect/DLTI/DLTIAttrs.cpp.inc"
489486
>();
490487
addInterfaces<TargetDataLayoutInterface, SystemDescSpecInterface>();
491488
}

‎mlir/lib/Interfaces/DataLayoutInterfaces.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -684,7 +684,8 @@ std::optional<uint32_t> mlir::DataLayout::getMaxVectorOpWidth(
684684
if (originalTargetSystemDesc) {
685685
if (auto device =
686686
originalTargetSystemDesc.getDeviceSpecForDeviceID(deviceID))
687-
entry = device->getSpecForMaxVectorOpWidth();
687+
entry =
688+
device->getSpecForIdentifier(device->getMaxVectorOpWidthIdentifier());
688689
}
689690
// Currently I am not caching the results because we do not return
690691
// default values of these properties. Instead if the property is
@@ -703,7 +704,8 @@ std::optional<uint32_t> mlir::DataLayout::getL1CacheSizeInBytes(
703704
if (originalTargetSystemDesc) {
704705
if (auto device =
705706
originalTargetSystemDesc.getDeviceSpecForDeviceID(deviceID))
706-
entry = device->getSpecForL1CacheSizeInBytes();
707+
entry = device->getSpecForIdentifier(
708+
device->getL1CacheSizeInBytesIdentifier());
707709
}
708710
// Currently I am not caching the results because we do not return
709711
// default values of these properties. Instead if the property is
@@ -831,7 +833,6 @@ mlir::detail::verifyTargetSystemSpec(TargetSystemSpecInterface spec,
831833
return failure();
832834

833835
// Check that device IDs are unique across all entries.
834-
MLIRContext *context = tdd_spec.getContext();
835836
TargetSystemSpecInterface::DeviceID device_id = entry.first;
836837
if (!device_ids.insert(device_id).second) {
837838
return failure();
@@ -842,8 +843,9 @@ mlir::detail::verifyTargetSystemSpec(TargetSystemSpecInterface spec,
842843
if (auto type = llvm::dyn_cast_if_present<Type>(entry.getKey())) {
843844
// tdd_spec does not support Type as a key.
844845
return failure();
845-
} else
846+
} else {
846847
device_desc_keys[entry.getKey().get<StringAttr>()] = entry;
848+
}
847849
}
848850
}
849851

‎mlir/test/Dialect/DLTI/roundtrip.mlir

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,12 @@
5656

5757
// A valid target system description
5858
// CHECK: module attributes {
59-
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
60-
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
61-
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>,
62-
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
63-
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>>
64-
// CHECK-SAME: >} {
59+
// CHECK: dlti.target_system_spec = #dlti.target_system_spec<
60+
// CHECK: "CPU" : #dlti.target_device_spec<
61+
// CHECK: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>,
62+
// CHECK: "GPU" : #dlti.target_device_spec<
63+
// CHECK: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>>
64+
// CHECK: >} {
6565
// CHECK: }
6666
module attributes {
6767
dlti.target_system_spec = #dlti.target_system_spec<

‎mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,7 @@ struct DLTestDialect : Dialect {
367367
}
368368
};
369369

370+
/// A dialect to test DLTI's target system spec and related attributes
370371
struct DLTargetSystemDescTestDialect : public Dialect {
371372
MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(DLTargetSystemDescTestDialect)
372373

0 commit comments

Comments
 (0)