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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst added a comment.

Nice! Thanks Hanhan! I just have a few nits. Feel free to land after addressing them.



================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:149
 
+/// Returns the shifted 32-bit value with the given offset.
+Value shiftStoreValue(Location loc, StoreOpOperandAdaptor op, Value offset,
----------------
Not necessarily 32-bit here?


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:150
+/// Returns the shifted 32-bit value with the given offset.
+Value shiftStoreValue(Location loc, StoreOpOperandAdaptor op, Value offset,
+                      Value mask, int targetBits, OpBuilder &builder) {
----------------
No need to require the whole `StoreOpOperandAdaptor` here given we just need `op.value()` in this function? Just passing in a `Value`? That way it creates cleaner interface.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:305
 
+/// Converts std.store to spv.Store.
+class IntStoreOpPattern final : public SPIRVOpLowering<StoreOp> {
----------------
The comment needs to be updated. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79272