[PATCH] D79272: [mlir][StandardToSPIRV] Emulate bitwidths not supported for store op.

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added a comment.
This revision now requires changes to proceed.

@antiagainst PTAL at the suggestion here.

@hanchung : Thanks for porting this to MLIR core. This was what we had converged on in IREE where we were trying to avoid the expensive compare exchange. We tried this, but looking at it with fresh eyes this seems buggy.



================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:732
+  int srcBits = memrefType.getElementType().getIntOrFloatBitWidth();
+  auto dstType = typeConverter.convertType(memrefType)
+                     .cast<spirv::PointerType>()
----------------
Might be easier to just convert the memref element type using the type converter (and do the same at the loadOp) instead of this.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:748-758
+  // Since there are multi threads in the processing, the emulation will be done
+  // with atomic operations. E.g., if the storing value is i8, rewrite the
+  // StoreOp to
+  // 1) load a 32-bit integer
+  // 2) clear 8 bits in the loading value
+  // 3) store 32-bit value back
+  // 4) load a 32-bit integer
----------------
I think these steps will not give the result that you want in all cases. There will be a race between the other threads that are modifying the same word, but different bits in the word. I dont see a way around it apart from using a atomic compare exchange.


do {
  %val = load %basePtr[%baseOffset] : !spv.ptr<i32, ..>
  %clearbits = and %val, %mask : i32 // Clear the bits that need to be updated.
  %updateval = or %clearbits, %update : i32 // Update the bits as required
} while( %val != atomicCompareExchange(%basePtr, %updateVal, %val));

See semantics of compareExchange here : https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#OpAtomicCompareExchange. If the condition of the while failed, it implies the compareExchange succeeded, i.e. the value at %basePtr was same as the %val that was loaded. If the condition failed, then the value at %basePtr was different from what was loaded indicating a stale load (due to an other write to the same word).





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79272