[PowerPC] Fix up memory ordering after combining BV to a load

[nemanjai]

Summary

The combiner for BUILD_VECTOR that merges consecutive loads into a wide load had two issues:

• It didn't check that the input loads all have the same input chain
• It didn't update nodes that are chained to the original loads to be chained to the new load

This caused issues with bootstrap when 3c4d2a03968ccf5889bacffe02d6fa2443b0260f was committed. This patch fixes the issue so it can unblock this commit.

[lebedev.ri]

@nemanjai thank you *so* much for dealing with this miscompile!

[RKSimon]

LGTM, although its annoying that you can't use SelectionDAG::areNonVolatileConsecutiveLoads somehow.

One minor - nothing to do with this patch, but something that probably should be addressed if you're in the vicinity.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

14280–14281

You're immediately dereferencing dyn_cast<> - these can probably be cast<>?

[efriedma]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

14263

Do we really need a SmallPtrSet here? We check each load exactly once, so we should be able to just use a SmallVector.

Iterating over a SmallPtrSet like this might lead to non-determinism.

[nemanjai]

In D140046#3996275, @RKSimon wrote:

LGTM, although its annoying that you can't use SelectionDAG::areNonVolatileConsecutiveLoads somehow.

One minor - nothing to do with this patch, but something that probably should be addressed if you're in the vicinity.

I think we just missed SelectionDAG::areNonVolatileConsecutiveLoads in the initial implementation. We should be able to use that, but I can refactor that in a follow-up.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

14263

Sounds good. I'll change it to SmallVector.

14280–14281

Sure, I can change that while I'm here.

[nemanjai]

Addressed review comments:

• Switch from SmallPtrSet to SmallVector
• Used areNonVolatileConsecutiveLoads() and updated it to consider the memory VT vs. the loaded VT
• Switched dyn_cast to cast for converting nodes whose opcodes were already checked

[nemanjai]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

11500

@efriedma @RKSimon Do you see any issues with switching this? It seems that we should be checking the memory type to determine if loads are consecutive. This is also required in order to use this query for extending loads for PPC.

[nemanjai]

Forgot to remove the redundant checks for input chain equality now that I switched to areNonVolatileConsecutiveLoads().

[efriedma]

LGTM

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

11500

I think all the existing uses exclude extending loads? Seems fine.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

14309

Is LD1 here the same node as LD above? Could simplify that a bit, maybe.

[lebedev.ri]

Ship it! :)

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

11500

an assertion/earlyout for extend loads might make sense?

[lebedev.ri]

YAY! Thank you!
I'm going to reland SROA patch, hopefully all bots stay happy this time :)

[lebedev.ri]

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

[lebedev.ri]

In D140046#4002169, @lebedev.ri wrote:

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

Silver-lining: little-endian PPC bots appear to be green: https://lab.llvm.org/buildbot/#/builders/121 https://lab.llvm.org/buildbot/#/builders/36
So the remaining problem is limited to big-endian.

[nemanjai]

In D140046#4002193, @lebedev.ri wrote:

In D140046#4002169, @lebedev.ri wrote:

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

Silver-lining: little-endian PPC bots appear to be green: https://lab.llvm.org/buildbot/#/builders/121 https://lab.llvm.org/buildbot/#/builders/36
So the remaining problem is limited to big-endian.

@lebedev.ri Did your original commit have this problem on the big endian bot?

[lebedev.ri]

In D140046#4002234, @nemanjai wrote:

In D140046#4002193, @lebedev.ri wrote:

In D140046#4002169, @lebedev.ri wrote:

@nemanjai looks like i may have some rather unfortunate news:
https://lab.llvm.org/buildbot/#/builders/93/builds/12560
I'm waiting for https://lab.llvm.org/buildbot/#/builders/93/builds/12561 to finish, and if it suddenly doesn't succeed, revert the recommit...

Silver-lining: little-endian PPC bots appear to be green: https://lab.llvm.org/buildbot/#/builders/121 https://lab.llvm.org/buildbot/#/builders/36
So the remaining problem is limited to big-endian.

@lebedev.ri Did your original commit have this problem on the big endian bot?

Yup: https://lab.llvm.org/buildbot/#/builders/93/builds/12251
To be honest, i didn't see that particular report :/

[nemanjai]

In D140046#4002236, @lebedev.ri wrote:

@lebedev.ri Did your original commit have this problem on the big endian bot?

Yup: https://lab.llvm.org/buildbot/#/builders/93/builds/12251
To be honest, i didn't see that particular report :/

OK, give me a couple of hours to see if I can track this down. At least I know what the likely culprit is so it's worth looking there. If I can't fix it right away, I'll revert your patch, get the PPC back end fixed and let you know.

[lebedev.ri]

In D140046#4002241, @nemanjai wrote:

In D140046#4002236, @lebedev.ri wrote:

@lebedev.ri Did your original commit have this problem on the big endian bot?

Yup: https://lab.llvm.org/buildbot/#/builders/93/builds/12251
To be honest, i didn't see that particular report :/

OK, give me a couple of hours to see if I can track this down. At least I know what the likely culprit is so it's worth looking there. If I can't fix it right away, I'll revert your patch, get the PPC back end fixed and let you know.

Thank you for taking a look!
I'll revert the patch regardless for now, looks like i need to fix commit message there.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

if (LD->isVolatile() || Base->isVolatile())
     return false;
   // TODO: probably too restrictive for atomics, revisit
   if (!LD->isSimple())
     return false;
   if (LD->isIndexed() || Base->isIndexed())
     return false;
   if (LD->getChain() != Base->getChain())
     return false;
-  EVT VT = LD->getValueType(0);
+  EVT VT = LD->getMemoryVT();
   if (VT.getSizeInBits() / 8 != Bytes)
     return false;
 
   auto BaseLocDecomp = BaseIndexOffset::match(Base, *this);
   auto LocDecomp = BaseIndexOffset::match(LD, *this);
 
   int64_t Offset = 0;
   if (BaseLocDecomp.equalBaseIndex(LocDecomp, *this, Offset))

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

static SDValue combineBVOfConsecutiveLoads(SDNode *N, SelectionDAG &DAG) {
   if (!N->getValueType(0).getVectorElementType().isByteSized())
     return SDValue();
 
   bool InputsAreConsecutiveLoads = true;
   bool InputsAreReverseConsecutive = true;
   unsigned ElemSize = N->getValueType(0).getScalarType().getStoreSize();
   SDValue FirstInput = N->getOperand(0);
   bool IsRoundOfExtLoad = false;
+  LoadSDNode *LD = nullptr;
 
   if (FirstInput.getOpcode() == ISD::FP_ROUND &&
       FirstInput.getOperand(0).getOpcode() == ISD::LOAD) {
-    LoadSDNode *LD = dyn_cast<LoadSDNode>(FirstInput.getOperand(0));
+    LD = cast<LoadSDNode>(FirstInput.getOperand(0));
     IsRoundOfExtLoad = LD->getExtensionType() == ISD::EXTLOAD;
   }
   // Not a build vector of (possibly fp_rounded) loads.
   if ((!IsRoundOfExtLoad && FirstInput.getOpcode() != ISD::LOAD) ||
       N->getNumOperands() == 1)
     return SDValue();
 
+  if (!IsRoundOfExtLoad)
+    LD = cast<LoadSDNode>(FirstInput);
+
+  SmallVector<LoadSDNode *, 4> InputLoads;
+  InputLoads.push_back(LD);
   for (int i = 1, e = N->getNumOperands(); i < e; ++i) {
     // If any inputs are fp_round(extload), they all must be.
     if (IsRoundOfExtLoad && N->getOperand(i).getOpcode() != ISD::FP_ROUND)
       return SDValue();
 
     SDValue NextInput = IsRoundOfExtLoad ? N->getOperand(i).getOperand(0) :
       N->getOperand(i);
     if (NextInput.getOpcode() != ISD::LOAD)
       return SDValue();
 
     SDValue PreviousInput =
       IsRoundOfExtLoad ? N->getOperand(i-1).getOperand(0) : N->getOperand(i-1);
-    LoadSDNode *LD1 = dyn_cast<LoadSDNode>(PreviousInput);
-    LoadSDNode *LD2 = dyn_cast<LoadSDNode>(NextInput);
+    LoadSDNode *LD1 = cast<LoadSDNode>(PreviousInput);
+    LoadSDNode *LD2 = cast<LoadSDNode>(NextInput);
 
     // If any inputs are fp_round(extload), they all must be.
     if (IsRoundOfExtLoad && LD2->getExtensionType() != ISD::EXTLOAD)
       return SDValue();
 
-    if (!isConsecutiveLS(LD2, LD1, ElemSize, 1, DAG))
+    // We only care about regular loads. The PPC-specific load intrinsics
+    // will not lead to a merge opportunity.
+    if (!DAG.areNonVolatileConsecutiveLoads(LD2, LD1, ElemSize, 1))
       InputsAreConsecutiveLoads = false;
-    if (!isConsecutiveLS(LD1, LD2, ElemSize, 1, DAG))
+    if (!DAG.areNonVolatileConsecutiveLoads(LD1, LD2, ElemSize, 1))
       InputsAreReverseConsecutive = false;
 
     // Exit early if the loads are neither consecutive nor reverse consecutive.
     if (!InputsAreConsecutiveLoads && !InputsAreReverseConsecutive)
       return SDValue();
+    InputLoads.push_back(LD2);
   }
 
   assert(!(InputsAreConsecutiveLoads && InputsAreReverseConsecutive) &&
          "The loads cannot be both consecutive and reverse consecutive.");
 
   SDValue FirstLoadOp =
     IsRoundOfExtLoad ? FirstInput.getOperand(0) : FirstInput;
   SDValue LastLoadOp =
     IsRoundOfExtLoad ? N->getOperand(N->getNumOperands()-1).getOperand(0) :
                        N->getOperand(N->getNumOperands()-1);
 
   LoadSDNode *LD1 = dyn_cast<LoadSDNode>(FirstLoadOp);
   LoadSDNode *LDL = dyn_cast<LoadSDNode>(LastLoadOp);
+  SDValue WideLoad;
+  SDValue ReturnSDVal;
   if (InputsAreConsecutiveLoads) {
     assert(LD1 && "Input needs to be a LoadSDNode.");
-    return DAG.getLoad(N->getValueType(0), dl, LD1->getChain(),
-                       LD1->getBasePtr(), LD1->getPointerInfo(),
-                       LD1->getAlign());
-  }
-  if (InputsAreReverseConsecutive) {
+    WideLoad =
+        DAG.getLoad(N->getValueType(0), dl, LD1->getChain(), LD1->getBasePtr(),
+                    LD1->getPointerInfo(), LD1->getAlign());
+    ReturnSDVal = WideLoad;
+  } else if (InputsAreReverseConsecutive) {
     assert(LDL && "Input needs to be a LoadSDNode.");
-    SDValue Load =
+    WideLoad =
         DAG.getLoad(N->getValueType(0), dl, LDL->getChain(), LDL->getBasePtr(),
                     LDL->getPointerInfo(), LDL->getAlign());
     SmallVector<int, 16> Ops;
     for (int i = N->getNumOperands() - 1; i >= 0; i--)
       Ops.push_back(i);
 
-    return DAG.getVectorShuffle(N->getValueType(0), dl, Load,
+    ReturnSDVal = DAG.getVectorShuffle(N->getValueType(0), dl, WideLoad,
                                        DAG.getUNDEF(N->getValueType(0)), Ops);
-  }
+  } else
     return SDValue();
+
+  for (auto *LD : InputLoads)
+    DAG.makeEquivalentMemoryOrdering(LD, WideLoad);
+  return ReturnSDVal;
 }
 
 // This function adds the required vector_shuffle needed to get
 // the elements of the vector extract in the correct position
 // as specified by the CorrectElems encoding.
 static SDValue addShuffleForVecExtend(SDNode *N, SelectionDAG &DAG,
                                       SDValue Input, uint64_t Elems,
                                       uint64_t CorrectElems) {

llvm/test/CodeGen/PowerPC/build-vector-to-ld-chain.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-- -mcpu=pwr8 < %s | \
+; RUN:   FileCheck %s
+
+%0 = type <{ %1, ptr, i32, [4 x i8] }>
+%1 = type { %2 }
+%2 = type { %3 }
+%3 = type { ptr, ptr, ptr }
+
+$testfunc = comdat any
+
+declare void @_ZdlPv() local_unnamed_addr #0
+
+define void @testfunc(i64 %arg) local_unnamed_addr #0 comdat {
+; CHECK-LABEL: testfunc:
+; CHECK:       # %bb.0: # %bb
+; CHECK-NEXT:    mflr 0
+; CHECK-NEXT:    stdu 1, -80(1)
+; CHECK-NEXT:    std 0, 96(1)
+; CHECK-NEXT:    .cfi_def_cfa_offset 80
+; CHECK-NEXT:    .cfi_offset lr, 16
+; CHECK-NEXT:    .cfi_offset v30, -32
+; CHECK-NEXT:    .cfi_offset v31, -16
+; CHECK-NEXT:    li 4, 48
+; CHECK-NEXT:    addi 3, 3, 24
+; CHECK-NEXT:    stvx 30, 1, 4 # 16-byte Folded Spill
+; CHECK-NEXT:    li 4, 64
+; CHECK-NEXT:    stvx 31, 1, 4 # 16-byte Folded Spill
+; CHECK-NEXT:    lxvd2x 63, 0, 3
+; CHECK-NEXT:    xxswapd 62, 63
+; CHECK-NEXT:    bc 12, 20, .LBB0_2
+; CHECK-NEXT:  # %bb.1: # %bb37
+; CHECK-NEXT:    bl _ZdlPv
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  .LBB0_2: # %bb38
+; CHECK-NEXT:    stxsiwx 62, 0, 3
+; CHECK-NEXT:    stxsdx 63, 0, 3
+; CHECK-NEXT:    li 3, 64
+; CHECK-NEXT:    lvx 31, 1, 3 # 16-byte Folded Reload
+; CHECK-NEXT:    li 3, 48
+; CHECK-NEXT:    lvx 30, 1, 3 # 16-byte Folded Reload
+; CHECK-NEXT:    addi 1, 1, 80
+; CHECK-NEXT:    ld 0, 16(1)
+; CHECK-NEXT:    mtlr 0
+; CHECK-NEXT:    blr
+bb:
+  %i = inttoptr i64 %arg to ptr
+  %i6 = getelementptr inbounds %0, ptr %i, i64 0, i32 1
+  %i7 = load <12 x i8>, ptr %i6, align 8
+  br i1 poison, label %bb38, label %bb37
+
+bb37:                                             ; preds = %bb
+  tail call void @_ZdlPv() #1
+  br label %bb38
+
+bb38:                                             ; preds = %bb37, %bb
+  store <12 x i8> %i7, ptr poison, align 8
+  ret void
+}