[PR27390] [CodeGen] Reject indexed loads in CombinerDAG.

[• koriakin]

Summary

visitAND, when folding and (load) forgets to check which output of
an indexed load is involved, happily folding the updated address
output on the following testcase:

target datalayout = "e-m:e-i64:64-n32:64"
target triple = "powerpc64le-unknown-linux-gnu"

%typ = type { i32, i32 }

define signext i32 @_Z8access_pP1Tc(%typ* %p, i8 zeroext %type) {

%b = getelementptr inbounds %typ, %typ* %p, i64 0, i32 1
%1 = load i32, i32* %b, align 4
%2 = ptrtoint i32* %b to i64
%3 = and i64 %2, -35184372088833
%4 = inttoptr i64 %3 to i32*
%_msld = load i32, i32* %4, align 4
%zzz = add i32 %1,  %_msld
ret i32 %zzz

}

Fix this by checking ResNo.

I've found a few more places that currently neglect to check for
indexed load, and tightened them up as well, but I don't have test
cases for them. In fact, they might not be triggerable at all,
at least with current targets. Still, better safe than sorry.

[• koriakin]

Ping?

[hfinkel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3102 ↗

(On Diff #54013)

It is not clear this is necessary; the code below explicitly handles indexed loads:

if (Load->getExtensionType() == ISD::EXTLOAD) {
  NewLoad = DAG.getLoad(Load->getAddressingMode(), ISD::ZEXTLOAD,
                        Load->getValueType(0), SDLoc(Load),
                        Load->getChain(), Load->getBasePtr(),
                        Load->getOffset(), Load->getMemoryVT(),
                        Load->getMemOperand());
  // Replace uses of the EXTLOAD with the new ZEXTLOAD.
  if (Load->getNumValues() == 3) {
    // PRE/POST_INC loads have 3 values.
    SDValue To[] = { NewLoad.getValue(0), NewLoad.getValue(1),
                     NewLoad.getValue(2) };
    CombineTo(Load, To, 3, true);

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6965 ↗

(On Diff #54013)

Why?

[• koriakin]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3102 ↗

(On Diff #54013)

Ah, indeed it does. So it's OK to allow indexed loads here, but we'll need to check if N0.getResNo() is 0. I'll update the patch.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6965 ↗

(On Diff #54013)

The only caller (DAGCombiner::CombineConsecutiveLoads) doesn't support indexed loads, and I don't see any other place where they're checked for, but I don't actually have any example code that could trigger a problem here.

[• koriakin]

This version checks ResNo in visitAnd instead of disallowing indexed loads entirely.

[hfinkel]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6964 ↗

(On Diff #54858)

Okay. Please check in DAGCombiner::CombineConsecutiveLoads instead. Otherwise, LGTM.

[• koriakin]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6964 ↗

(On Diff #54858)

Hmm, I'm not sure the logic in areNonVolatileConsecutiveLoads works right in the presence of indexed load anyhow (in particular, pre-indexing needs to be taken into account). I'll take a look.

[• koriakin]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6964 ↗

(On Diff #54858)

Right, areNonVolatileConsecutiveLoads is incorrect for pre-indexed loads - it only takes getOperand(1) into account. So even if some caller can handle the indexing, it'll provide wrong answers.

[hfinkel]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6964 ↗

(On Diff #54858)

Fair enough. This is fine as-is then. Thanks!

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

void DAGCombiner::ReplaceLoadWithPromotedLoad(SDNode *Load, SDNode *ExtLoad) {
   DAG.ReplaceAllUsesOfValueWith(SDValue(Load, 1), SDValue(ExtLoad, 1));
   deleteAndRecombine(Load);
   AddToWorklist(Trunc.getNode());
 }
 
 SDValue DAGCombiner::PromoteOperand(SDValue Op, EVT PVT, bool &Replace) {
   Replace = false;
   SDLoc dl(Op);
-  if (LoadSDNode *LD = dyn_cast<LoadSDNode>(Op)) {
+  if (ISD::isUNINDEXEDLoad(Op.getNode())) {
+    LoadSDNode *LD = cast<LoadSDNode>(Op);
     EVT MemVT = LD->getMemoryVT();
     ISD::LoadExtType ExtType = ISD::isNON_EXTLoad(LD)
       ? (TLI.isLoadExtLegal(ISD::ZEXTLOAD, PVT, MemVT) ? ISD::ZEXTLOAD
                                                        : ISD::EXTLOAD)
       : LD->getExtensionType();
     Replace = true;
     return DAG.getExtLoad(ExtType, dl, PVT,
                           LD->getChain(), LD->getBasePtr(),
SDValue DAGCombiner::PromoteExtend(SDValue Op) {
   }
   return SDValue();
 }
 
 bool DAGCombiner::PromoteLoad(SDValue Op) {
   if (!LegalOperations)
     return false;
 
+  if (!ISD::isUNINDEXEDLoad(Op.getNode()))
+    return false;
+
   EVT VT = Op.getValueType();
   if (VT.isVector() || !VT.isInteger())
     return false;
 
   // If operation type is 'undesirable', e.g. i16 on x86, consider
   // promoting it.
   unsigned Opc = Op.getOpcode();
   if (TLI.isTypeDesirableForOp(Opc, VT))
SDValue DAGCombiner::visitAND(SDNode *N) {
   // similarly fold (and (X (load ([non_ext|any_ext|zero_ext] V))), c) ->
   // (X (load ([non_ext|zero_ext] V))) if 'and' only clears top bits which must
   // already be zero by virtue of the width of the base type of the load.
   //
   // the 'X' node here can either be nothing or an extract_vector_elt to catch
   // more cases.
   if ((N0.getOpcode() == ISD::EXTRACT_VECTOR_ELT &&
        N0.getValueSizeInBits() == N0.getOperand(0).getScalarValueSizeInBits() &&
-       N0.getOperand(0).getOpcode() == ISD::LOAD) ||
-      N0.getOpcode() == ISD::LOAD) {
+       N0.getOperand(0).getOpcode() == ISD::LOAD &&
+       N0.getOperand(0).getResNo() == 0) ||
+      (N0.getOpcode() == ISD::LOAD && N0.getResNo() == 0)) {
     LoadSDNode *Load = cast<LoadSDNode>( (N0.getOpcode() == ISD::LOAD) ?
                                          N0 : N0.getOperand(0) );
 
     // Get the constant (if applicable) the zero'th operand is being ANDed with.
     // This can be a pure constant or a vector splat, in which case we treat the
     // vector as a scalar and use the splat value.
     APInt Constant = APInt::getNullValue(1);
     if (const ConstantSDNode *C = dyn_cast<ConstantSDNode>(N1)) {

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

 }
 
 bool SelectionDAG::areNonVolatileConsecutiveLoads(LoadSDNode *LD,
                                                   LoadSDNode *Base,
                                                   unsigned Bytes,
                                                   int Dist) const {
   if (LD->isVolatile() || Base->isVolatile())
     return false;
+  if (LD->isIndexed() || Base->isIndexed())
+    return false;
   if (LD->getChain() != Base->getChain())
     return false;
   EVT VT = LD->getValueType(0);
   if (VT.getSizeInBits() / 8 != Bytes)
     return false;
 
   SDValue Loc = LD->getOperand(1);
   SDValue BaseLoc = Base->getOperand(1);

llvm/trunk/test/CodeGen/PowerPC/2016-04-17-combine.ll

+; RUN: llc <%s | FileCheck %s
+target datalayout = "e-m:e-i64:64-n32:64"
+target triple = "powerpc64le-unknown-linux-gnu"
+
+; PR27390 crasher
+
+%typ = type { i32, i32 }
+
+; On release builds, it doesn't crash, spewing nonsense instead.
+; To make sure it works, check that and is still alive.
+; CHECK: and
+; Also, in release, it emits a COPY from a 32-bit register to
+; a 64-bit register, which happens to be emitted as cror [!]
+; by the confused CodeGen.  Just to be sure, check there isn't one.
+; CHECK-NOT: cror
+; Function Attrs: uwtable
+define signext i32 @_Z8access_pP1Tc(%typ* %p, i8 zeroext %type) {
+  %b = getelementptr inbounds %typ, %typ* %p, i64 0, i32 1
+  %1 = load i32, i32* %b, align 4
+  %2 = ptrtoint i32* %b to i64
+  %3 = and i64 %2, -35184372088833
+  %4 = inttoptr i64 %3 to i32*
+  %_msld = load i32, i32* %4, align 4
+  %zzz = add i32 %1,  %_msld
+  ret i32 %zzz
+}