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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst marked an inline comment as done.
antiagainst added a comment.

Sorry didn't see Mahesh's comment before commenting. :)



================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:732
+  int srcBits = memrefType.getElementType().getIntOrFloatBitWidth();
+  auto dstType = typeConverter.convertType(memrefType)
+                     .cast<spirv::PointerType>()
----------------
mravishankar wrote:
> Might be easier to just convert the memref element type using the type converter (and do the same at the loadOp) instead of this.
This was requested by me. Actually depending on the storage class, we might want different capabilities, which determines whether we can use 16-bit or not in that particular storage class. For example, 16-bit in storage buffer is fine if we have `StorageBuffer16BitAccess`, but 16-bit in push constant is not fine if we don't have `StoragePushConstant16`. So we need to go through  the memref here.


================
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
----------------
mravishankar wrote:
> 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).
> 
> 
> 
I might be missing something but the approach here should be fine given each thread is actually only touching strictly disjoint portion of the word? Could you elaborate a bit here regarding the issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79272