Skip to content

Commit 5bc3886

Browse files
nhasabninhasabni
committed
Addressing review comments; adding unit tests
1 parent e3cfa7f commit 5bc3886

File tree

10 files changed

+418
-43
lines changed

10 files changed

+418
-43
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,9 +172,9 @@ def DLTI_TargetSystemDescSpecAttr :
172172
let extraClassDefinition = [{
173173
std::optional<TargetDeviceDescSpecInterface>
174174
$cppClass::getDeviceDescForDeviceID(
175-
TargetDeviceDescSpecInterface::DeviceID DeviceID) {
175+
TargetDeviceDescSpecInterface::DeviceID deviceID) {
176176
for (TargetDeviceDescSpecInterface entry : getEntries()) {
177-
if (entry.getDeviceID() == DeviceID)
177+
if (entry.getDeviceID() == deviceID)
178178
return entry;
179179
}
180180
return std::nullopt;

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,12 @@ def TargetDeviceDescSpecInterface : AttrInterface<"TargetDeviceDescSpecInterface
253253
InterfaceMethod<
254254
/*description=*/"Returns the L1 cache size identifier.",
255255
/*retTy=*/"::mlir::StringAttr",
256+
/*methodName=*/"getL1CacheSizeInBytesIdentifier",
257+
/*args=*/(ins)
258+
>,
259+
InterfaceMethod<
260+
/*description=*/"Returns the max vector op width identifier.",
261+
/*retTy=*/"::mlir::StringAttr",
256262
/*methodName=*/"getMaxVectorOpWidthIdentifier",
257263
/*args=*/(ins)
258264
>,

‎mlir/lib/Conversion/AMDGPUToROCDL/AMDGPUToROCDL.cpp

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
#include "mlir/Dialect/LLVMIR/LLVMDialect.h"
1616
#include "mlir/Dialect/LLVMIR/ROCDLDialect.h"
1717
#include "mlir/IR/BuiltinTypes.h"
18-
#include "mlir/IR/Operation.h"
1918
#include "mlir/IR/TypeUtilities.h"
2019
#include "mlir/Pass/Pass.h"
2120

@@ -30,8 +29,6 @@ namespace mlir {
3029
using namespace mlir;
3130
using namespace mlir::amdgpu;
3231

33-
#define DEBUG_TYPE "amd-gpu-to-rocdl"
34-
3532
static Value createI32Constant(ConversionPatternRewriter &rewriter,
3633
Location loc, int32_t value) {
3734
Type llvmI32 = rewriter.getI32Type();
@@ -52,6 +49,7 @@ struct RawBufferOpLowering : public ConvertOpToLLVMPattern<GpuOp> {
5249
: ConvertOpToLLVMPattern<GpuOp>(converter), chipset(chipset) {}
5350

5451
Chipset chipset;
52+
static constexpr uint32_t maxVectorOpWidth = 128;
5553

5654
LogicalResult
5755
matchAndRewrite(GpuOp gpuOp, typename GpuOp::Adaptor adaptor,
@@ -113,16 +111,6 @@ struct RawBufferOpLowering : public ConvertOpToLLVMPattern<GpuOp> {
113111
if (auto dataVector = dyn_cast<VectorType>(wantedDataType)) {
114112
uint32_t elemBits = dataVector.getElementTypeBitWidth();
115113
uint32_t totalBits = elemBits * dataVector.getNumElements();
116-
uint32_t maxVectorOpWidth = 128; // default value
117-
ModuleOp moduleOp = gpuOp->template getParentOfType<mlir::ModuleOp>();
118-
std::optional<uint32_t> v = std::nullopt;
119-
if (moduleOp &&
120-
(v = DataLayout(moduleOp).getMaxVectorOpWidth(1 /* gpu ID*/))) {
121-
maxVectorOpWidth = *v;
122-
}
123-
LLVM_DEBUG(llvm::dbgs() << "[CostModel] GPU MaxVectorWidth:"
124-
<< maxVectorOpWidth << "\n");
125-
126114
if (totalBits > maxVectorOpWidth)
127115
return gpuOp.emitOpError(
128116
"Total width of loads or stores must be no more than " +

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -302,8 +302,21 @@ TargetDeviceDescSpecAttr::verify(function_ref<InFlightDiagnostic()> emitError,
302302
if (auto value = llvm::dyn_cast<StringAttr>(entry.getValue())) {
303303
targetDeviceTypeKeyPresentAndValid = true;
304304
}
305-
} else if (entryName != DLTIDialect::kTargetDeviceMaxVectorOpWidthKey &&
306-
entryName != DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey) {
305+
} else if (entryName == DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey) {
306+
IntegerAttr value =
307+
llvm::dyn_cast_if_present<IntegerAttr>(entry.getValue());
308+
if (!value || !value.getType().isUnsignedInteger(32))
309+
return emitError() << "target_device_desc_spec requires value of key: "
310+
<< DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey
311+
<< " to be of ui32 type";
312+
} else if (entryName == DLTIDialect::kTargetDeviceMaxVectorOpWidthKey) {
313+
IntegerAttr value =
314+
llvm::dyn_cast_if_present<IntegerAttr>(entry.getValue());
315+
if (!value || !value.getType().isUnsignedInteger(32))
316+
return emitError() << "target_device_desc_spec requires value of key: "
317+
<< DLTIDialect::kTargetDeviceMaxVectorOpWidthKey
318+
<< " to be of ui32 type";
319+
} else {
307320
return emitError() << "unknown target device desc key name: "
308321
<< entryName;
309322
}

‎mlir/lib/Dialect/Linalg/Transforms/BlockPackMatmul.cpp

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@
1818

1919
#include <optional>
2020

21-
#define DEBUG_TYPE "block-pack-matmul"
22-
2321
namespace mlir {
2422
#define GEN_PASS_DEF_LINALGBLOCKPACKMATMUL
2523
#include "mlir/Dialect/Linalg/Passes.h.inc"
@@ -136,24 +134,6 @@ transposePackedMatmul(RewriterBase &rewriter, linalg::LinalgOp linalgOp,
136134
return packTransposedMatmul;
137135
}
138136

139-
static SmallVector<int64_t> getDefaultBlockFactors(linalg::LinalgOp linalgOp) {
140-
// get L1 cache size first.
141-
uint32_t L1_cache_size = 4096; // default value
142-
uint32_t cpuID = 0;
143-
ModuleOp moduleOp = linalgOp->getParentOfType<ModuleOp>();
144-
if (std::optional<int64_t> v =
145-
DataLayout(moduleOp).getL1CacheSizeInBytes(cpuID)) {
146-
L1_cache_size = *v;
147-
}
148-
149-
// block_size = sqrt(L1_cache_size) rounded down to nearest power of 2.
150-
int64_t block_size =
151-
std::pow(2, std::floor(std::log2(std::sqrt(L1_cache_size))));
152-
// we use same block size for all dims.
153-
LLVM_DEBUG(llvm::dbgs() << "block_size:" << block_size << "\n");
154-
return {block_size, block_size, block_size};
155-
}
156-
157137
/// Pack a matmul operation into blocked 4D layout.
158138
FailureOr<PackResult>
159139
linalg::blockPackMatmul(RewriterBase &rewriter, linalg::LinalgOp linalgOp,
@@ -166,7 +146,7 @@ linalg::blockPackMatmul(RewriterBase &rewriter, linalg::LinalgOp linalgOp,
166146
return rewriter.notifyMatchFailure(linalgOp, "invalid packing options");
167147

168148
if (options->blockFactors.size() != 3)
169-
options->blockFactors = getDefaultBlockFactors(linalgOp);
149+
return rewriter.notifyMatchFailure(linalgOp, "require 3 tile factors");
170150

171151
SmallVector<OpFoldResult> mnkTiles =
172152
getAsOpFoldResult(rewriter.getI64ArrayAttr(options->blockFactors));

‎mlir/lib/Transforms/Canonicalizer.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ namespace mlir {
2323

2424
using namespace mlir;
2525

26-
#define DEBUG_TYPE "canonicalizer"
27-
2826
namespace {
2927
/// Canonicalize operations in nested regions.
3028
struct Canonicalizer : public impl::CanonicalizerBase<Canonicalizer> {

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

Lines changed: 130 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636

3737
// -----
3838

39-
// expected-error@below {{unknown attrribute type: unknown}}
39+
// expected-error@below {{unknown attribute `unknown` in dialect `dlti`}}
4040
"test.unknown_op"() { test.unknown_attr = #dlti.unknown } : () -> ()
4141

4242
// -----
@@ -90,3 +90,132 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<"unknown.unknown
9090
// expected-note@above {{enclosing op with data layout}}
9191
"test.op_with_data_layout"() { dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<"unknown.unknown", 32>>} : () -> ()
9292
}
93+
94+
// -----
95+
96+
// expected-error@below {{'dlti.target_system_desc_spec' is expected to be a #dlti.target_system_desc_spec attribute}}
97+
"test.unknown_op"() { dlti.target_system_desc_spec = 42 } : () -> ()
98+
99+
// -----
100+
101+
// expected-error@below {{invalid kind of attribute specified}}
102+
"test.unknown_op"() { dlti.target_system_desc_spec = #dlti.target_system_desc_spec<[]> } : () -> ()
103+
104+
// -----
105+
106+
module attributes {
107+
// expected-error@+2 {{target_device_desc_spec requires key: dlti.device_id and its value of ui32 type}}
108+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
109+
#dlti.target_device_desc_spec<
110+
#dlti.dl_entry<"dlti.device_type", "CPU">>
111+
>} {}
112+
113+
// -----
114+
115+
module attributes {
116+
// expected-error@+2 {{target_device_desc_spec requires key: dlti.device_type and its value of string type}}
117+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
118+
#dlti.target_device_desc_spec<
119+
#dlti.dl_entry<"dlti.device_id", 0: ui32>>
120+
>} {}
121+
122+
// -----
123+
124+
module attributes {
125+
// expected-error@+2 {{target_device_desc_spec requires key: dlti.device_id and its value of ui32 type}}
126+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
127+
#dlti.target_device_desc_spec<
128+
#dlti.dl_entry<"dlti.device_id", 0: i32>>
129+
>} {}
130+
131+
// -----
132+
133+
module attributes {
134+
// expected-error@+2 {{target_device_desc_spec requires key: dlti.device_type and its value of string type}}
135+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
136+
#dlti.target_device_desc_spec<
137+
#dlti.dl_entry<"dlti.device_id", 0 : ui32>,
138+
#dlti.dl_entry<"dlti.device_type", 0: i32>>
139+
>} {}
140+
141+
// -----
142+
143+
module attributes {
144+
// expected-error@+2 {{repeated layout entry key: dlti.device_id}}
145+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
146+
#dlti.target_device_desc_spec<
147+
#dlti.dl_entry<"dlti.device_id", 0 : ui32>,
148+
#dlti.dl_entry<"dlti.device_id", 1 : ui32>,
149+
#dlti.dl_entry<"dlti.device_type", "CPU">,
150+
#dlti.dl_entry<"dlti.L1_cache_size", 4096 : i32>>
151+
>} {}
152+
153+
// -----
154+
155+
module attributes {
156+
// expected-error@+2 {{repeated layout entry key: dlti.device_type}}
157+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
158+
#dlti.target_device_desc_spec<
159+
#dlti.dl_entry<"dlti.device_id", 0 : ui32>,
160+
#dlti.dl_entry<"dlti.device_type", "CPU">,
161+
#dlti.dl_entry<"dlti.device_type", "GPU">,
162+
#dlti.dl_entry<"dlti.L1_cache_size", 4096 : i32>>
163+
>} {}
164+
165+
// -----
166+
167+
module attributes {
168+
// expected-error@+2 {{target_device_desc_spec requires value of key: dlti.L1_cache_size_in_bytes to be of ui32 type}}
169+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
170+
#dlti.target_device_desc_spec<
171+
#dlti.dl_entry<"dlti.device_id", 0 : ui32>,
172+
#dlti.dl_entry<"dlti.device_type", "CPU">,
173+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096.1 : f32>>
174+
>} {}
175+
176+
// -----
177+
178+
module attributes {
179+
// expected-error@+2 {{target_device_desc_spec requires value of key: dlti.max_vector_op_width to be of ui32 type}}
180+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
181+
#dlti.target_device_desc_spec<
182+
#dlti.dl_entry<"dlti.device_id", 0 : ui32>,
183+
#dlti.dl_entry<"dlti.device_type", "CPU">,
184+
#dlti.dl_entry<"dlti.max_vector_op_width", 4096.1 : f32>>
185+
>} {}
186+
187+
// -----
188+
189+
module attributes {
190+
// expected-error@+2 {{unknown target device desc key name: dlti.L2_cache_size_in_bytes}}
191+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
192+
#dlti.target_device_desc_spec<
193+
#dlti.dl_entry<"dlti.device_id", 0 : ui32>,
194+
#dlti.dl_entry<"dlti.device_type", "CPU">,
195+
#dlti.dl_entry<"dlti.L2_cache_size_in_bytes", 4096 : i32>>
196+
>} {}
197+
198+
// -----
199+
200+
module attributes {
201+
// expected-error@+2 {{unknown target device desc key name: dlti.unknown_key}}
202+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
203+
#dlti.target_device_desc_spec<
204+
#dlti.dl_entry<"dlti.device_id", 0>,
205+
#dlti.dl_entry<"dlti.device_type", "CPU">,
206+
#dlti.dl_entry<"dlti.unknown_key", 42>>
207+
>} {}
208+
209+
// -----
210+
211+
module attributes {
212+
// unexpected-error@below {{repeated Device ID in dlti.target_system_desc_spec: 0}}
213+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
214+
#dlti.target_device_desc_spec<
215+
#dlti.dl_entry<"dlti.device_id", 0: ui32>,
216+
#dlti.dl_entry<"dlti.device_type", "CPU">>,
217+
#dlti.target_device_desc_spec<
218+
#dlti.dl_entry<"dlti.device_id", 0: ui32>,
219+
#dlti.dl_entry<"dlti.device_type", "CPU">>
220+
>} {}
221+

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,3 +53,29 @@
5353
}) { dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<"unknown.unknown", 32>> } : () -> ()
5454
"test.maybe_terminator_op"() : () -> ()
5555
}) { dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<"unknown.unknown", 32>> } : () -> ()
56+
57+
// A valid target system description
58+
// CHECK: module attributes {
59+
// CHECK-SAME: dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
60+
// CHECK-SAME: #dlti.target_device_desc_spec<
61+
// CHECK-SAME: #dlti.dl_entry<"dlti.device_id", 0 : ui32>,
62+
// CHECK-SAME: #dlti.dl_entry<"dlti.device_type", "CPU">,
63+
// CHECK-SAME: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>,
64+
// CHECK-SAME: #dlti.target_device_desc_spec<
65+
// CHECK-SAME: #dlti.dl_entry<"dlti.device_id", 1 : ui32>,
66+
// CHECK-SAME: #dlti.dl_entry<"dlti.device_type", "GPU">,
67+
// CHECK-SAME: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>>
68+
// CHECK-SAME: >} {
69+
// CHECK: }
70+
module attributes {
71+
dlti.target_system_desc_spec = #dlti.target_system_desc_spec<
72+
#dlti.target_device_desc_spec<
73+
#dlti.dl_entry<"dlti.device_id", 0: ui32>,
74+
#dlti.dl_entry<"dlti.device_type", "CPU">,
75+
#dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096: ui32>>,
76+
#dlti.target_device_desc_spec<
77+
#dlti.dl_entry<"dlti.device_id", 1: ui32>,
78+
#dlti.dl_entry<"dlti.device_type", "GPU">,
79+
#dlti.dl_entry<"dlti.max_vector_op_width", 128: ui32>>
80+
>} {}
81+

0 commit comments

Comments
 (0)