[PATCH] D78974: [mlir][StandardToSPIRV] Emulate bitwidths not supported for load op.

[Han-Chung Wang via Phabricator via llvm-commits]

hanchung marked an inline comment as done.
hanchung added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:129
+  auto idx = rewriter.create<spirv::ConstantOp>(
+      loc, i32Type, rewriter.getI32IntegerAttr(32 / bits));
+  auto lastDim = op.getOperation()->getOperand(op.getNumOperands() - 1);
----------------
antiagainst wrote:
> assert llvm::isPowerOf2_32(bits)?
I make it to handle any target integer width, add `assert(targetBits % elementBits == 0);` in the beginning.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:553
+    // Assume that it is unconditionally converted to i32 type.
+    assert(convertedBits == 32);
+    assert(convertedBits % bits == 0);
----------------
mravishankar wrote:
> Could we add a smarter logic here. We can try to find the "next highest power of 2" that is legal and use that instead.
This depends on how type converter handles it. In this case, I followed your suggestion to generalize it -- making all 32-bit to convertedBit. Thus, if the typeConverter does try to find the "next highest power of 2", it will still work.


================
Comment at: mlir/test/Conversion/StandardToSPIRV/std-ops-to-spirv.mlir:626
+// CHECK-LABEL: @load
+func @load(%arg0: memref<i8>, %arg1: memref<10xi16>, %arg2: memref<i32>,
+           %arg3: memref<f32>) {
----------------
antiagainst wrote:
> antiagainst wrote:
> > What about creating separate functions for each type so that we have more focused and easier-to-read tests?
> We will need tests with `StorageBuffer16BitAcess`/etc. capability.
That's what I think after sent out for review...I planned to fix it in a later rivision.


================
Comment at: mlir/test/Conversion/StandardToSPIRV/std-ops-to-spirv.mlir:626
+// CHECK-LABEL: @load
+func @load(%arg0: memref<i8>, %arg1: memref<10xi16>, %arg2: memref<i32>,
+           %arg3: memref<f32>) {
----------------
hanchung wrote:
> antiagainst wrote:
> > antiagainst wrote:
> > > What about creating separate functions for each type so that we have more focused and easier-to-read tests?
> > We will need tests with `StorageBuffer16BitAcess`/etc. capability.
> That's what I think after sent out for review...I planned to fix it in a later rivision.
I added one more test, although the exts and caps are more than I expected. Please let me know if this isn't the case you'd like me to add. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78974/new/

https://reviews.llvm.org/D78974