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

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:103
+// are four elements in one i32, and one element has 8 bits.
+static Value getOffsetOfInt(spirv::AccessChainOp op, int bits,
+                            ConversionPatternRewriter &rewriter) {
----------------
Couple of things here
1) This assumes bits < 32. Probably need to assert that as well.
2) It would be nice to actually not specialize this to 32-bits. You could take the target integer type as an argument and the same logic should more or less hold.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:123
+/// works for a scalar or 1-D tensor.
+static Value convertToI32AccessChain(SPIRVTypeConverter &typeConverter,
+                                     spirv::AccessChainOp op, int bits,
----------------
This too could be generalized to handle any target integer width.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:539
+
+  int bits = memrefType.getElementType().getIntOrFloatBitWidth();
+  Type convertedType = typeConverter.convertType(memrefType.getElementType());
----------------
This will assert if this is not an integer. So it might be better to have a different pattern for load stores when the memref is integer type. So one pattern will implement this logic for integer type load/stores. Another pattern will be generic that will be type agnostic (and will return failure for integer types to not intersect with the other pattern)


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78974