[DAGCombine] Initialize the bytes offset vector as INT64_MAX to avoid inference the endian check

[steven.zhang]

Summary

This is a regression of https://reviews.llvm.org/D61843 found by the build bot. This is the narrow down case:

void foo(char *p, char v) {
        p[0] = v;
        p[1] = v;
}

We have a table ByteOffsets to map the offset in the combined value "v"(the vector index) to the offset in the store address "p" (the vector element value).

In this case, we will have:

ByteOffsets[0] = 0;     // The first byte of "v" is stored to p[0]              
ByteOffsets[0] = 1;     // The first byte of "v" is stored to p[1]

Therefore, at last, ByteOffsets[0] will be set as "1". However, as this vector is created with 2 elements that has "0" as its element default value, therefore, it will look like this:

ByteOffsets[0] = 1; // As described before.
ByteOffsets[1] = 0; // The ctor initialized it as 0.

And this match the endian check, so, we will do the transformation, which is wrong.

Therefore, we need to initialize the default value of the table as INT64_MAX to avoid this issue. In fact, the range of the value is, [-63, 63], as it is offset from the base ("p"). And we can only merge 64 stores at most.

I have run the check-all, llvm testsuite, spec2017, and bootstrap, all are clean.

[RKSimon]

Do we need an assert anywhere to ensure that the ByteOffsets element has been set to something else?

[steven.zhang]

We cannot have the assertion, as it is possible that, it isn't set to something else(for example, this case). And the isBigEndian() check will fail if it isn't set to something else. However, this is a good point, we could do a check when each element is set, early return if it has been set before.

[steven.zhang]

Address comments. Early return if it has been set before, to make the code more robust.

[RKSimon]

LGTM cheers

[steven.zhang]

@RKSimon Thank you for your nice review!

[steven.zhang]

recommit the patch https://reviews.llvm.org/D61843 together with this one.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::MatchStoreCombine(StoreSDNode *N) {
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   if (LegalOperations && !TLI.isOperationLegal(ISD::STORE, VT))
     return SDValue();
 
   // Check if all the bytes of the combined value we are looking at are stored 
   // to the same base address. Collect bytes offsets from Base address into 
   // ByteOffsets. 
   SDValue CombinedValue;
-  SmallVector<int64_t, 4> ByteOffsets(Width);
+  SmallVector<int64_t, 4> ByteOffsets(Width, INT64_MAX);
   int64_t FirstOffset = INT64_MAX;
   StoreSDNode *FirstStore = nullptr;
   Optional<BaseIndexOffset> Base;
   for (auto Store : Stores) {
     // All the stores store different byte of the CombinedValue. A truncate is
     // required to get that byte value.
     SDValue Trunc = Store->getValue();
     if (Trunc.getOpcode() != ISD::TRUNCATE)

llvm/test/CodeGen/PowerPC/store-combine.ll

entry:
   %conv1 = trunc i16 %m to i8
   store i8 %conv1, i8* %p, align 1
   %0 = lshr i16 %m, 8
   %conv5 = trunc i16 %0 to i8
   %arrayidx6 = getelementptr inbounds i8, i8* %p, i64 1
   store i8 %conv5, i8* %arrayidx6, align 1
   ret void
 }
+; This was found when testing the hexxagon in testsuite
+; i8* p; i8 v;
+; p[0] = v;
+; p[1] = v;
+define void @store_same_value_to_consecutive_mem(i8* %p, i8 zeroext %v) {
+; CHECK-PPC64LE-LABEL: store_same_value_to_consecutive_mem 
+; CHECK-PPC64LE:       # %bb.0: # %entry 
+; CHECK-PPC64LE-NEXT:    stb 4, 0(3) 
+; CHECK-PPC64LE-NEXT:    stb 4, 1(3) 
+;
+; CHECK-PPC64-LABEL: store_same_value_to_consecutive_mem 
+; CHECK-PPC64:       # %bb.0: # %entry
+; CHECK-PPC64-NEXT:    stb 4, 0(3)
+; CHECK-PPC64-NEXT:    stb 4, 1(3)
+entry:
+  store i8 %v, i8* %p, align 1
+  %arrayidx1 = getelementptr inbounds i8, i8* %p, i64 1
+  store i8 %v, i8* %arrayidx1, align 1
+  ret void
+}