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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added a comment.

Nice! I just have a few more nits.



================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:100
 
+/// Returns the offset of input value in `targetBits` integer representation.
+/// For example, if `elementBits` equals to 8 and `targetBits` equals to 32, the
----------------
What about:

Assuming `index` is an index into a 1-D array with each element having `sourceBits`, returns the adjusted `index` by treating the 1-D array as having elements of `targetBits`? 

This means renaming `lastDim` to `index`. 


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:104
+/// one i32, and one element has 8 bits.
+static Value getOffsetOfInt(Location loc, Value lastDim, int elementBits,
+                            int targetBits, OpBuilder &builder) {
----------------
Sorry for nitpicking again, but with `targetBits`, it's better to call `elementBits` as `sourceBits` then. ;) Simlarly for the next function.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:104
+/// one i32, and one element has 8 bits.
+static Value getOffsetOfInt(Location loc, Value lastDim, int elementBits,
+                            int targetBits, OpBuilder &builder) {
----------------
antiagainst wrote:
> Sorry for nitpicking again, but with `targetBits`, it's better to call `elementBits` as `sourceBits` then. ;) Simlarly for the next function.
What about naming it as `adjust1DArrayIndexForBitwidth`? It's nothing special to integer anymore. 


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:122
+/// only works for a scalar or 1-D tensor.
+static Value convertToTargetAccessChain(SPIRVTypeConverter &typeConverter,
+                                        spirv::AccessChainOp op,
----------------
What about naming it as `adjustAccessChainForBitwidth`?


================
Comment at: mlir/test/Conversion/StandardToSPIRV/std-ops-to-spirv.mlir:654
+// CHECK-LABEL: @load_i16
+func @load_i16(%arg0: memref<i16>) {
+  //     CHECK: %[[ZERO:.+]] = spv.constant 0 : i32
----------------
It would be nice to test a 1-D memref here and with a index coming as function parameter. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78974