[PATCH] D79753: [mlir][StandardToSPIRV] Fix a bug in loading a non-i32 value.

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

hanchung created this revision.
hanchung added reviewers: antiagainst, denis13, mravishankar.
Herald added subscribers: llvm-commits, Kayjukh, frgossen, grosul1, bader, Joonsoo, stephenneuendorffer, liufengdb, lucyrfox, mgester, arpith-jacob, nicolasvasilache, shauheen, jpienaar, rriddle, mehdi_amini.
Herald added a project: LLVM.
hanchung added inline comments.


================
Comment at: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp:665
+
+  // If the number is negative, we should do a sign extention.
+  IntegerAttr shiftValueAttr =
----------------
It seems that we should check the signedness semantics of dstType?

This looks like we should apply following sign extension if `srcType.isSignedInteger()` returns true, and shouldn't apply it if `srcType.isUnsignedInteger()` returns true. In case of it is signless, ie `srcType.isSignlessInteger()` returns true, I don't know what should we do. It seems an undef to me.


Previously, after applying the mask, a negative number would convert to a
positive number because the sign flag was forgotten. This patch adds two more
shift operations to do the sign extension. This assumes that we're using two's
complement.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79753

Files:
  mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
  mlir/test/Conversion/StandardToSPIRV/std-ops-to-spirv.mlir


Index: mlir/test/Conversion/StandardToSPIRV/std-ops-to-spirv.mlir
===================================================================
--- mlir/test/Conversion/StandardToSPIRV/std-ops-to-spirv.mlir
+++ mlir/test/Conversion/StandardToSPIRV/std-ops-to-spirv.mlir
@@ -689,7 +689,10 @@
   //     CHECK: %[[BITS:.+]] = spv.IMul %[[IDX]], %[[EIGHT]] : i32
   //     CHECK: %[[VALUE:.+]] = spv.ShiftRightArithmetic %[[LOAD]], %[[BITS]] : i32, i32
   //     CHECK: %[[MASK:.+]] = spv.constant 255 : i32
-  //     CHECK: spv.BitwiseAnd %[[VALUE]], %[[MASK]] : i32
+  //     CHECK: %[[T1:.+]] = spv.BitwiseAnd %[[VALUE]], %[[MASK]] : i32
+  //     CHECK: %[[T2:.+]] = spv.constant 24 : i32
+  //     CHECK: %[[T3:.+]] = spv.ShiftLeftLogical %[[T1]], %[[T2]] : i32, i32
+  //     CHECK: spv.ShiftRightArithmetic %[[T3]], %[[T2]] : i32, i32
   %0 = load %arg0[] : memref<i8>
   return
 }
@@ -710,7 +713,10 @@
   //     CHECK: %[[BITS:.+]] = spv.IMul %[[IDX]], %[[SIXTEEN]] : i32
   //     CHECK: %[[VALUE:.+]] = spv.ShiftRightArithmetic %[[LOAD]], %[[BITS]] : i32, i32
   //     CHECK: %[[MASK:.+]] = spv.constant 65535 : i32
-  //     CHECK: spv.BitwiseAnd %[[VALUE]], %[[MASK]] : i32
+  //     CHECK: %[[T1:.+]] = spv.BitwiseAnd %[[VALUE]], %[[MASK]] : i32
+  //     CHECK: %[[T2:.+]] = spv.constant 16 : i32
+  //     CHECK: %[[T3:.+]] = spv.ShiftLeftLogical %[[T1]], %[[T2]] : i32, i32
+  //     CHECK: spv.ShiftRightArithmetic %[[T3]], %[[T2]] : i32, i32
   %0 = load %arg0[%index] : memref<10xi16>
   return
 }
@@ -824,7 +830,10 @@
   //     CHECK: %[[BITS:.+]] = spv.IMul %[[IDX]], %[[EIGHT]] : i32
   //     CHECK: %[[VALUE:.+]] = spv.ShiftRightArithmetic %[[LOAD]], %[[BITS]] : i32, i32
   //     CHECK: %[[MASK:.+]] = spv.constant 255 : i32
-  //     CHECK: spv.BitwiseAnd %[[VALUE]], %[[MASK]] : i32
+  //     CHECK: %[[T1:.+]] = spv.BitwiseAnd %[[VALUE]], %[[MASK]] : i32
+  //     CHECK: %[[T2:.+]] = spv.constant 24 : i32
+  //     CHECK: %[[T3:.+]] = spv.ShiftLeftLogical %[[T1]], %[[T2]] : i32, i32
+  //     CHECK: spv.ShiftRightArithmetic %[[T3]], %[[T2]] : i32, i32
   %0 = load %arg0[] : memref<i8>
   return
 }
Index: mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
===================================================================
--- mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
+++ mlir/lib/Conversion/StandardToSPIRV/ConvertStandardToSPIRV.cpp
@@ -661,6 +661,16 @@
   Value mask = rewriter.create<spirv::ConstantOp>(
       loc, dstType, rewriter.getIntegerAttr(dstType, (1 << srcBits) - 1));
   result = rewriter.create<spirv::BitwiseAndOp>(loc, dstType, result, mask);
+
+  // If the number is negative, we should do a sign extention.
+  IntegerAttr shiftValueAttr =
+      rewriter.getIntegerAttr(dstType, dstBits - srcBits);
+  Value shiftValue =
+      rewriter.create<spirv::ConstantOp>(loc, dstType, shiftValueAttr);
+  result = rewriter.create<spirv::ShiftLeftLogicalOp>(loc, dstType, result,
+                                                      shiftValue);
+  result = rewriter.create<spirv::ShiftRightArithmeticOp>(loc, dstType, result,
+                                                          shiftValue);
   rewriter.replaceOp(loadOp, result);
 
   assert(accessChainOp.use_empty());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D79753.263310.patch
Type: text/x-patch
Size: 3248 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200512/126c8430/attachment.bin>