[SelectionDAG] Combine extload/store/load sequence into extload.

[ab]

Summary

If this extload is directly stored, and that store's input value was
loaded before still, try to avoid the roundtrip through memory and
extload directly from the original memory location.

This can't be caught by the other store-to-load forwarding DAGCombines,
because the extload's memory type might not be legal.
Instead, try to match this:

v4i32 load %ptr2, zext from v4i8
  store i32 %value, %ptr2
    %value = load i32 %ptr1

to turn the extload into:

v4i32 load %ptr1, zext from v4i8

Avoids a regression in an X86 testcase with D6533.

[qcolombet]

Hi Ahmed,

LGTM.

Thanks,
-Quentin

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8314

IIRC, we tend to write the chain of computation in the opposite order:
load
store
extload

Please make sure you use the same convention here and I did not remember correctly, then ignore this :).

8317

!ISD::isNON_EXTLoad(N) => ISD::isEXTLoad(N)
That may be less contrived.

[ab]

Before committing, a few more questions, inline.

Thanks for the review!
-Ahmed

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8314

You're right, I was corrupted by SDNode::dumpr() :P

8317

The name is misleading: isEXTLoad checks for "anyext-loads", not "any kind of extload"!

8323

Should we also check that the types are equivalent? For instance, if you have

%v = load float %p1
store float %v, %p2
%v2 = i64 load %p2, zext from i32

would it be a problem? (I don't think so, but I'm having second thoughts.)

8329

I think we can remove this check: we don't really care that the alignment is the same, do we? As long as the new load has the same alignment as the first (non-extending) one.

[qcolombet]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

8317

Right :).

8323

As long as we read the same number of bits, this is fine.
You already check for that :).

8329

Indeed, we do not need to check that the alignment is the same.

As long as the new load has the same alignment as the first (non-extending) one.

I do not think we need to check for that. Nevertheless, we should check that the alignment for the new load (derived from the load of %p1) is enough to be legal.

Does it make sense?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (ISD::isNormalLoad(N) && !LD->isVolatile()) {
     if (ISD::isNON_TRUNCStore(Chain.getNode())) {
       StoreSDNode *PrevST = cast<StoreSDNode>(Chain);
       if (PrevST->getBasePtr() == Ptr &&
           PrevST->getValue().getValueType() == N->getValueType(0))
       return CombineTo(N, Chain.getOperand(1), Chain);
     }
   }
 
+  // If this extload is directly stored, and that store's input value was
+  // loaded before still, try to avoid the roundtrip through memory and
+  // extload directly from the original memory location.
+  //
+  // This can't be caught by the store-to-load forwarding above, because
+  // the extload's memory type might not be legal.
+  // Instead, try to match this:
+  // v4i32 load %ptr2, zext from v4i8
+  //   store i32 %value, %ptr2
+  //     %value = load i32 %ptr1
+  // to turn the extload into:
+  // v4i32 load %ptr1, zext from v4i8
+  if (!ISD::isNON_EXTLoad(N) && !LD->isVolatile() &&
+      ISD::isNON_TRUNCStore(Chain.getNode())) {
+    SDLoc DL(N);
+    StoreSDNode *PrevST = cast<StoreSDNode>(Chain);
+    EVT ValVT = PrevST->getValue().getValueType();
+    EVT MemVT = LD->getMemoryVT();
+    if (PrevST->getBasePtr() == Ptr &&
+        (ValVT.getStoreSize() == MemVT.getStoreSize() &&
+         ValVT.getSizeInBits() == MemVT.getSizeInBits()) &&
+        ISD::isNormalLoad(PrevST->getValue().getNode())) {
+      LoadSDNode *PPrevLD = cast<LoadSDNode>(PrevST->getValue());
+      if (LD->getAddressSpace() == PPrevLD->getAddressSpace() &&
+          LD->getAlignment() == PPrevLD->getAlignment()) {
+        SDValue NewLoad = DAG.getExtLoad(
+            LD->getExtensionType(), DL, LD->getValueType(0),
+            PPrevLD->getChain(), PPrevLD->getBasePtr(),
+            PPrevLD->getPointerInfo(), MemVT, /*isVolatile=*/false,
+            PPrevLD->isNonTemporal(), PPrevLD->isInvariant(),
+            PPrevLD->getAlignment(), PPrevLD->getAAInfo());
+        SDValue Token = DAG.getNode(ISD::TokenFactor, DL, MVT::Other, Chain,
+                                    SDValue(NewLoad.getNode(), 1));
+        return CombineTo(N, NewLoad, Token, true);
+      }
+    }
+  }
+
   // Try to infer better alignment information than the load already has.
   if (OptLevel != CodeGenOpt::None && LD->isUnindexed()) {
     if (unsigned Align = DAG.InferPtrAlignment(Ptr)) {
       if (Align > LD->getMemOperand()->getBaseAlignment()) {
         SDValue NewLoad =
                DAG.getExtLoad(LD->getExtensionType(), SDLoc(N),
                               LD->getValueType(0),
                               Chain, Ptr, LD->getPointerInfo(),

test/CodeGen/X86/widen_load-2.ll

 ; CHECK-NEXT:    movw    %[[R0]]x, (%[[PTR0:.*]])
 ; CHECK-NEXT:    movb    $-98, 2(%[[PTR0]])
 ; CHECK-NEXT:    movdqa  {{.*}}, %[[CONSTANT1:xmm[0-9]+]]
 ; CHECK-NEXT:    pshufb  %[[SHUFFLE_MASK]], %[[CONSTANT1]]
 ; CHECK-NEXT:    pmovzxwq %[[CONSTANT1]], %[[CONSTANT1]]
 ; CHECK-NEXT:    movd    %[[CONSTANT1]], %e[[R1:[abcd]]]x
 ; CHECK-NEXT:    movw    %[[R1]]x, (%[[PTR1:.*]])
 ; CHECK-NEXT:    movb    $1, 2(%[[PTR1]])
-; CHECK-NEXT:    movl    (%[[PTR0]]), [[TMP1:%e[abcd]+x]]
-; CHECK-NEXT:    movl    [[TMP1]], [[TMP2:.*]]
-; CHECK-NEXT:    pmovzxbd [[TMP2]], %[[X0:xmm[0-9]+]]
+; CHECK-NEXT:    pmovzxbd (%[[PTR0]]), %[[X0:xmm[0-9]+]]
 ; CHECK-NEXT:    pextrd  $1, %[[X0]], %e[[R0:[abcd]]]x
 ; CHECK-NEXT:    shrl    %e[[R0]]x
 ; CHECK-NEXT:    movd    %[[X0]], %e[[R1:[abcd]]]x
 ; CHECK-NEXT:    shrl    %e[[R1]]x
 ; CHECK-NEXT:    movd    %e[[R1]]x, %[[X1:xmm[0-9]+]]
 ; CHECK-NEXT:    pinsrd  $1, %e[[R0]]x, %[[X1]]
 ; CHECK-NEXT:    pextrd  $2, %[[X0]], %e[[R0:[abcd]]]x
 ; CHECK-NEXT:    shrl    %e[[R0]]x