Skip to content

Commit 856426f

Browse files
nhasabninhasabni
committed
Addressing review comments
1 parent 87d1b20 commit 856426f

File tree

7 files changed

+95
-41
lines changed

7 files changed

+95
-41
lines changed

‎mlir/include/mlir/Interfaces/DataLayoutInterfaces.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,11 +93,11 @@ uint64_t getDefaultStackAlignment(DataLayoutEntryInterface entry);
9393

9494
/// Return max vector op width from the specified DataLayoutEntry. If the
9595
/// property is missing from the entry, then return std::nullopt.
96-
std::optional<uint32_t> getMaxVectorOpWidth(DataLayoutEntryInterface entry);
96+
std::optional<int64_t> getMaxVectorOpWidth(DataLayoutEntryInterface entry);
9797

9898
/// Return L1 cache size in bytes from the specified DataLayoutEntry. If the
9999
/// property is missing from the entry, then return std::nullopt.
100-
std::optional<uint32_t> getL1CacheSizeInBytes(DataLayoutEntryInterface entry);
100+
std::optional<int64_t> getL1CacheSizeInBytes(DataLayoutEntryInterface entry);
101101

102102
/// Given a list of data layout entries, returns a new list containing the
103103
/// entries with keys having the given type ID, i.e. belonging to the same type
@@ -249,12 +249,12 @@ class DataLayout {
249249

250250
/// Returns for max vector op width if the property is defined for the given
251251
/// device ID, otherwise return std::nullopt.
252-
std::optional<uint32_t>
252+
std::optional<int64_t>
253253
getMaxVectorOpWidth(TargetSystemSpecInterface::DeviceID) const;
254254

255255
/// Returns for L1 cache size if the property is defined for the given
256256
/// device ID, otherwise return std::nullopt.
257-
std::optional<uint32_t>
257+
std::optional<int64_t>
258258
getL1CacheSizeInBytes(TargetSystemSpecInterface::DeviceID) const;
259259

260260
private:

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,7 @@ def DataLayoutOpInterface : OpInterface<"DataLayoutOpInterface"> {
482482
StaticInterfaceMethod<
483483
/*description=*/"Returns the max vector op width, if the property is "
484484
"defined. Otherwise, it returns std::nullopt.",
485-
/*retTy=*/"std::optional<uint32_t>",
485+
/*retTy=*/"std::optional<int64_t>",
486486
/*methodName=*/"getMaxVectorOpWidth",
487487
/*args=*/(ins "::mlir::DataLayoutEntryInterface":$entry),
488488
/*methodBody=*/"",
@@ -493,7 +493,7 @@ def DataLayoutOpInterface : OpInterface<"DataLayoutOpInterface"> {
493493
StaticInterfaceMethod<
494494
/*description=*/"Returns the L1 cache size in bytes, if the property is "
495495
"defined. Otherwise, it returns std::nullopt.",
496-
/*retTy=*/"std::optional<uint32_t>",
496+
/*retTy=*/"std::optional<int64_t>",
497497
/*methodName=*/"getL1CacheSizeInBytes",
498498
/*args=*/(ins "::mlir::DataLayoutEntryInterface":$entry),
499499
/*methodBody=*/"",

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -362,17 +362,17 @@ TargetDeviceSpecAttr::verify(function_ref<InFlightDiagnostic()> emitError,
362362
if (entryName == DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey) {
363363
IntegerAttr value =
364364
llvm::dyn_cast_if_present<IntegerAttr>(entry.getValue());
365-
if (!value || !value.getType().isUnsignedInteger(32))
365+
if (!value || !value.getType().isInteger())
366366
return emitError() << "target_device_spec requires value of key: "
367367
<< DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey
368-
<< " to be of ui32 type";
368+
<< " to be of integer type";
369369
} else if (entryName == DLTIDialect::kTargetDeviceMaxVectorOpWidthKey) {
370370
IntegerAttr value =
371371
llvm::dyn_cast_if_present<IntegerAttr>(entry.getValue());
372-
if (!value || !value.getType().isUnsignedInteger(32))
372+
if (!value || !value.getType().isInteger())
373373
return emitError() << "target_device_spec requires value of key: "
374374
<< DLTIDialect::kTargetDeviceMaxVectorOpWidthKey
375-
<< " to be of ui32 type";
375+
<< " to be of integer type";
376376
} else {
377377
return emitError() << "unknown target device spec key name: "
378378
<< entryName;

‎mlir/lib/Interfaces/DataLayoutInterfaces.cpp

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -295,7 +295,7 @@ mlir::detail::getDefaultStackAlignment(DataLayoutEntryInterface entry) {
295295

296296
// Returns the max vector op width if specified in the given entry. If the entry
297297
// is empty (meaning the spec is missing), returns std::nullopt.
298-
std::optional<uint32_t>
298+
std::optional<int64_t>
299299
mlir::detail::getMaxVectorOpWidth(DataLayoutEntryInterface entry) {
300300
if (entry == DataLayoutEntryInterface())
301301
return std::nullopt;
@@ -306,7 +306,7 @@ mlir::detail::getMaxVectorOpWidth(DataLayoutEntryInterface entry) {
306306

307307
// Returns the L1 cache size if specified in the given entry. If the entry
308308
// is empty (meaning the spec is missing), returns std::nullopt.
309-
std::optional<uint32_t>
309+
std::optional<int64_t>
310310
mlir::detail::getL1CacheSizeInBytes(DataLayoutEntryInterface entry) {
311311
if (entry == DataLayoutEntryInterface())
312312
return std::nullopt;
@@ -468,21 +468,21 @@ void checkMissingLayout(DataLayoutSpecInterface originalLayout, OpTy op) {
468468
mlir::DataLayout::DataLayout() : DataLayout(ModuleOp()) {}
469469

470470
mlir::DataLayout::DataLayout(DataLayoutOpInterface op)
471-
: originalLayout(getCombinedDataLayout(op)), scope(op),
471+
: originalLayout(getCombinedDataLayout(op)),
472+
originalTargetSystemDesc(getTargetSystemSpec(op)), scope(op),
472473
allocaMemorySpace(std::nullopt), programMemorySpace(std::nullopt),
473-
globalMemorySpace(std::nullopt), stackAlignment(std::nullopt),
474-
originalTargetSystemDesc(getTargetSystemSpec(op)) {
474+
globalMemorySpace(std::nullopt), stackAlignment(std::nullopt) {
475475
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
476476
checkMissingLayout(originalLayout, op);
477477
collectParentLayouts(op, layoutStack);
478478
#endif
479479
}
480480

481481
mlir::DataLayout::DataLayout(ModuleOp op)
482-
: originalLayout(getCombinedDataLayout(op)), scope(op),
482+
: originalLayout(getCombinedDataLayout(op)),
483+
originalTargetSystemDesc(getTargetSystemSpec(op)), scope(op),
483484
allocaMemorySpace(std::nullopt), programMemorySpace(std::nullopt),
484-
globalMemorySpace(std::nullopt), stackAlignment(std::nullopt),
485-
originalTargetSystemDesc(getTargetSystemSpec(op)) {
485+
globalMemorySpace(std::nullopt), stackAlignment(std::nullopt) {
486486
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
487487
checkMissingLayout(originalLayout, op);
488488
collectParentLayouts(op, layoutStack);
@@ -677,7 +677,7 @@ uint64_t mlir::DataLayout::getStackAlignment() const {
677677
return *stackAlignment;
678678
}
679679

680-
std::optional<uint32_t> mlir::DataLayout::getMaxVectorOpWidth(
680+
std::optional<int64_t> mlir::DataLayout::getMaxVectorOpWidth(
681681
TargetSystemSpecInterface::DeviceID deviceID) const {
682682
checkValid();
683683
DataLayoutEntryInterface entry;
@@ -697,7 +697,7 @@ std::optional<uint32_t> mlir::DataLayout::getMaxVectorOpWidth(
697697
return detail::getMaxVectorOpWidth(entry);
698698
}
699699

700-
std::optional<uint32_t> mlir::DataLayout::getL1CacheSizeInBytes(
700+
std::optional<int64_t> mlir::DataLayout::getL1CacheSizeInBytes(
701701
TargetSystemSpecInterface::DeviceID deviceID) const {
702702
checkValid();
703703
DataLayoutEntryInterface entry;

‎mlir/test/Dialect/DLTI/invalid.mlir

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ module attributes {
113113
// expected-error@+2 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
114114
dlti.target_system_spec = #dlti.target_system_spec<
115115
: #dlti.target_device_spec<
116-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>
116+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : i32>>
117117
>} {}
118118

119119
// -----
@@ -137,17 +137,17 @@ module attributes {
137137
// expected-error@below {{repeated Device ID in dlti.target_system_spec: "CPU"}}
138138
dlti.target_system_spec = #dlti.target_system_spec<
139139
"CPU": #dlti.target_device_spec<
140-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>,
140+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096>>,
141141
"CPU": #dlti.target_device_spec<
142-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192 : ui32>>
142+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192>>
143143
>} {}
144144

145145
// -----
146146

147147
module attributes {
148148
// L1_cache_size_in_bytes is of incorrect type
149149
//
150-
// expected-error@+4 {{target_device_spec requires value of key: dlti.L1_cache_size_in_bytes to be of ui32 type}}
150+
// expected-error@+4 {{target_device_spec requires value of key: dlti.L1_cache_size_in_bytes to be of integer type}}
151151
// expected-error@+5 {{Error in parsing target device spec}}
152152
// expected-error@+4 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
153153
dlti.target_system_spec = #dlti.target_system_spec<
@@ -160,7 +160,7 @@ module attributes {
160160
module attributes {
161161
// max_vector_op_width is of incorrect type
162162
//
163-
// expected-error@+4 {{target_device_spec requires value of key: dlti.max_vector_op_width to be of ui32 type}}
163+
// expected-error@+4 {{target_device_spec requires value of key: dlti.max_vector_op_width to be of integer type}}
164164
// expected-error@+5 {{Error in parsing target device spec}}
165165
// expected-error@+4 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
166166
dlti.target_system_spec = #dlti.target_system_spec<
@@ -178,8 +178,8 @@ module attributes {
178178
// expected-error@+5 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
179179
dlti.target_system_spec = #dlti.target_system_spec<
180180
"CPU": #dlti.target_device_spec<
181-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>,
182-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192 : ui32>>
181+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096>,
182+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192>>
183183
>} {}
184184

185185
// -----

‎mlir/test/Dialect/DLTI/valid.mlir

Lines changed: 66 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,51 +5,105 @@
55
// CHECK: module attributes {
66
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
77
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
8-
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>,
8+
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : i32>>,
99
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
10-
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>>
10+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : i32>>
1111
// CHECK-SAME: >} {
1212
// CHECK: }
1313
module attributes {
1414
dlti.target_system_spec = #dlti.target_system_spec<
1515
"CPU": #dlti.target_device_spec<
16-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096: ui32>>,
16+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096: i32>>,
1717
"GPU": #dlti.target_device_spec<
18-
#dlti.dl_entry<"dlti.max_vector_op_width", 128: ui32>>
18+
#dlti.dl_entry<"dlti.max_vector_op_width", 128: i32>>
1919
>} {}
2020

2121
// -----
2222

2323
// CHECK: module attributes {
2424
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
2525
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
26-
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>,
26+
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : i32>>,
2727
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
28-
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192 : ui32>>
28+
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192 : i32>>
2929
// CHECK-SAME: >} {
3030
// CHECK: }
3131
module attributes {
3232
dlti.target_system_spec = #dlti.target_system_spec<
3333
"CPU": #dlti.target_device_spec<
34-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096: ui32>>,
34+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096: i32>>,
3535
"GPU": #dlti.target_device_spec<
36-
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192: ui32>>
36+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192: i32>>
3737
>} {}
3838

3939
// -----
4040

4141
// CHECK: module attributes {
4242
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
4343
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
44-
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 64 : ui32>>,
44+
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : i64>>,
4545
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
46-
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>>
46+
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192 : i64>>
4747
// CHECK-SAME: >} {
4848
// CHECK: }
4949
module attributes {
5050
dlti.target_system_spec = #dlti.target_system_spec<
5151
"CPU": #dlti.target_device_spec<
52-
#dlti.dl_entry<"dlti.max_vector_op_width", 64: ui32>>,
52+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096: i64>>,
5353
"GPU": #dlti.target_device_spec<
54-
#dlti.dl_entry<"dlti.max_vector_op_width", 128: ui32>>
54+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 8192: i64>>
55+
>} {}
56+
57+
// -----
58+
59+
// CHECK: module attributes {
60+
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
61+
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
62+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 64 : i32>>,
63+
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
64+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : i32>>
65+
// CHECK-SAME: >} {
66+
// CHECK: }
67+
module attributes {
68+
dlti.target_system_spec = #dlti.target_system_spec<
69+
"CPU": #dlti.target_device_spec<
70+
#dlti.dl_entry<"dlti.max_vector_op_width", 64: i32>>,
71+
"GPU": #dlti.target_device_spec<
72+
#dlti.dl_entry<"dlti.max_vector_op_width", 128: i32>>
73+
>} {}
74+
75+
// -----
76+
77+
// CHECK: module attributes {
78+
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
79+
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
80+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 64 : i64>>,
81+
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
82+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : i64>>
83+
// CHECK-SAME: >} {
84+
// CHECK: }
85+
module attributes {
86+
dlti.target_system_spec = #dlti.target_system_spec<
87+
"CPU": #dlti.target_device_spec<
88+
#dlti.dl_entry<"dlti.max_vector_op_width", 64: i64>>,
89+
"GPU": #dlti.target_device_spec<
90+
#dlti.dl_entry<"dlti.max_vector_op_width", 128: i64>>
91+
>} {}
92+
93+
// -----
94+
95+
// CHECK: module attributes {
96+
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
97+
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
98+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 64 : i64>>,
99+
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
100+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : i64>>
101+
// CHECK-SAME: >} {
102+
// CHECK: }
103+
module attributes {
104+
dlti.target_system_spec = #dlti.target_system_spec<
105+
"CPU": #dlti.target_device_spec<
106+
#dlti.dl_entry<"dlti.max_vector_op_width", 64>>,
107+
"GPU": #dlti.target_device_spec<
108+
#dlti.dl_entry<"dlti.max_vector_op_width", 128>>
55109
>} {}

‎mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -608,10 +608,10 @@ TEST(DataLayout, SpecWithTargetSystemDescEntries) {
608608
DataLayout layout(*module);
609609
EXPECT_EQ(layout.getL1CacheSizeInBytes(
610610
Builder(&ctx).getStringAttr("CPU") /* device ID*/),
611-
std::optional<uint32_t>(4096));
611+
std::optional<int64_t>(4096));
612612
EXPECT_EQ(layout.getMaxVectorOpWidth(
613613
Builder(&ctx).getStringAttr("CPU") /* device ID*/),
614-
std::optional<uint32_t>(128));
614+
std::optional<int64_t>(128));
615615
}
616616

617617
TEST(DataLayout, Caching) {

0 commit comments

Comments
 (0)