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

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar added inline comments.


================
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);
----------------
idx and elemBitsValue seem to be the same...


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:117
+
+/// Returns an adjusted spirv::AccessChainOp to access corresponding
+/// `targetBits` integer representation elements. One element was a
----------------
This comment is hard to parse. Maybe more descriptive will help. Something along the lines

Based on the extension/capabilities, certain integer bitwidths (`targetBits`) might not be supported. During conversion if a memref of an unsupported type is used, load/stores to this memref need to be modified to use a supported higher bitwidth (`elementBits`) and extracting the required bits. For a accessing a 1D array (spv.array or spv.rt_array), the last index is modified to load the bits needed. The extraction of the actual bits needed are handled separately.


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


================
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);
+}
----------------
use builder.replaceOpWithNewOp. I am assuming the older accesschain operation is dead and needs to be deleted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78974