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

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

hanchung added inline comments.


================
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
----------------
antiagainst wrote:
> 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`. 
I think the method is not to adjust the index. Instead, it's calculating the offset of value from loaded value. When accessing the value from target 1-D array, multiple values are loaded in the same time. In this context, the method returns the offset where the `srcIdx` locates in the value. In the example, it's (x % 4) * 8, not (x % 4).

I add more comments here, please take a look.


================
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:
> 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. 
Yes, sourceBits is better. thanks!


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:112
+  auto idx = builder.create<spirv::ConstantOp>(loc, targetType, attr);
+  auto elemBitsValue = builder.create<spirv::ConstantOp>(loc, targetType, attr);
+  auto m = builder.create<spirv::SModOp>(loc, lastDim, idx);
----------------
mravishankar wrote:
> idx and elemBitsValue seem to be the same...
Good catch, thanks!


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:135
+  // There are two elements if this is a 1-D tensor.
+  assert(indices.size() <= 2);
+  if (indices.size() > 1)
----------------
mravishankar wrote:
> This probably needs some explanation. If the accesschain is created while lowering a zero-rank memref, you have only one element in indices. You are just changing the element type here. This is still valid cause the host side would have to use the same bitwidth to store the scalar (Even though it needs lesser bitwidth).
I just found that there is no scalar case here because getElementPtr() always linearize the buffer. If it's a scalar, we still turn it to a 1D array.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:139
+  Type t = typeConverter.convertType(op.component_ptr().getType());
+  return builder.create<spirv::AccessChainOp>(loc, t, op.base_ptr(), indices);
+}
----------------
mravishankar wrote:
> use builder.replaceOpWithNewOp. I am assuming the older accesschain operation is dead and needs to be deleted.
The method looks more like returning an adjusted ptr to me, so we can focus more on how to build the ptr. I think keeping the replacement logic in the matchAndRewrite method is better. In this use case, we don't want it to be destroyed immediately because we still need some information from it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78974