[PATCH] D40790: DAGCombiner bugfix in MergeStoresOfConstantsOrVecElts()

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa updated this revision to Diff 125482.
jonpa added a comment.

Test case reduced further.

> It's not clear to me what it takes to reveal the bug, but it shouldn't be (much?) more than a series of stores. Can you reduce the test further than this?

thanks - yes I could get it down a bit further with bugpoint.

> Also, can FP constant stores have the same problem?

Good question... This would happen if the original stores were truncating wider FP constants. That seems much less likely to me, but I am not sure.

> Did this bug start after r319036?

This problem disappeared at least on the test case when I reverted that patch, so yes, this seems to be the cause (It also explains why I all of a sudden got some 10 csmith wrong-code results relating to this recently...)


https://reviews.llvm.org/D40790

Files:
  lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  test/CodeGen/SystemZ/DAGCombiner_stores.ll


Index: test/CodeGen/SystemZ/DAGCombiner_stores.ll
===================================================================
--- /dev/null
+++ test/CodeGen/SystemZ/DAGCombiner_stores.ll
@@ -0,0 +1,40 @@
+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=zEC12 < %s  | FileCheck %s
+;
+; This (reduced csmith) test case stores a lot of 16 bit constant -7 values
+; to adjacent locations, which the DAGCombiner will merge to wider stores. It
+; was discovered that the result of combining two or four such stores
+; resulted incorrectly in stores of 32-bit / 64-bit constants of value
+; -7. The correct constant then is not -7, but consists of two or four i16 -7
+; elements.
+
+; CHECK-NOT: mvhi {{.*}} -7
+
+ at y = external unnamed_addr global [10 x [7 x i16]]
+
+define void @bad_merge() {
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 0, i64 2), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 0, i64 3), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 0, i64 4), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 2, i64 1), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 2, i64 3), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 3, i64 0), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 3, i64 1), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 3, i64 6), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 4, i64 3), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 4, i64 4), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 4, i64 5), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 5, i64 0), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 5, i64 2), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 5, i64 4), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 5, i64 6), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 6, i64 1), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 6, i64 4), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 7, i64 1), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 7, i64 5), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 8, i64 0), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 8, i64 4), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 8, i64 6), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 9, i64 5), align 2
+  store i16 -7, i16* getelementptr inbounds ([10 x [7 x i16]], [10 x [7 x i16]]* @y, i64 0, i64 9, i64 6), align 2
+  ret void
+}
Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -12678,7 +12678,11 @@
       SDValue Val = St->getValue();
       StoreInt <<= ElementSizeBits;
       if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Val)) {
-        StoreInt |= C->getAPIntValue().zextOrTrunc(SizeInBits);
+        // Since the store may be truncating, the stored immediate must be
+        // truncated to the width of the element.
+        APInt CElt(ElementSizeBits,
+               C->getAPIntValue().zextOrTrunc(ElementSizeBits).getZExtValue());
+        StoreInt |= CElt.zextOrTrunc(SizeInBits);
       } else if (ConstantFPSDNode *C = dyn_cast<ConstantFPSDNode>(Val)) {
         StoreInt |= C->getValueAPF().bitcastToAPInt().zextOrTrunc(SizeInBits);
       } else {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D40790.125482.patch
Type: text/x-patch
Size: 4475 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171205/fef1346b/attachment.bin>