Cleanup Chain Handling in X86ISelLowering

[niravd]

Summary

Previously only users of the first merged load's chain were merged which
potentially allows the merged load to elided from the chain of nodes
chained off of another of the merged loads.

[spatel]

Can you expose the change in a test case?

[jyknight]

This looks wrong, as it's losing the *original* load from the chain now.

Uses of any of the original chain outputs should be modified to depend on BOTH the original load, and the new load (through a TokenFactor), because I don't believe we are actually necessarily removing the original load during these transforms. That is, I think the original code was correct and necessary, as far as it went -- the issue was only that it failed to do the exact same thing for the output chains of any but the first load.

Also, please reference the PR for this.

[niravd]

Uses of any of the original chain outputs should be modified to depend on BOTH the original load, and the new load (through a TokenFactor), because I don't believe we are actually necessarily removing the original load during these transforms. That is, I think the original code was correct and necessary, as far as it went -- the issue was only that it failed to do the exact same thing for the output chains of any but the first load.

Right. Since it's duplicated there's no reason to expect them to be evaluated the same, but this would drop any WAR hazard on those just loads.

[RKSimon]

Is this PR10114?

[RKSimon]

Nirav are you still looking at this? We just got stung by a similar problem in broadcasts (D25039).

[RKSimon]

@niravd Whatever happened to this patch?

[RKSimon]

@niravd Abandon now that D38547 has landed?

[niravd]

Already handled in D38547

lib/Target/X86/X86ISelLowering.cpp

if (LoadMask[i]) {
         break;
       }
     } else if (ZeroMask[i]) {
       IsConsecutiveLoad = false;
       break;
     }
   }
 
-  auto CreateLoad = [&DAG, &DL](EVT VT, LoadSDNode *LDBase) {
+  auto CreateLoad = [&DAG, &DL, NumElems, LoadMask, Elts](EVT VT,
+                                                          LoadSDNode *LDBase) {
     SDValue NewLd = DAG.getLoad(VT, DL, LDBase->getChain(),
                                 LDBase->getBasePtr(), LDBase->getPointerInfo(),
-                                LDBase->isVolatile(), LDBase->isNonTemporal(),
+                                false /*isVolatile*/, LDBase->isNonTemporal(),
                                 LDBase->isInvariant(), LDBase->getAlignment());
-
-    if (LDBase->hasAnyUseOfValue(1)) {
-      SDValue NewChain =
-          DAG.getNode(ISD::TokenFactor, DL, MVT::Other, SDValue(LDBase, 1),
-                      SDValue(NewLd.getNode(), 1));
-      DAG.ReplaceAllUsesOfValueWith(SDValue(LDBase, 1), NewChain);
-      DAG.UpdateNodeOperands(NewChain.getNode(), SDValue(LDBase, 1),
+    // all references to chain for each component load should now point
+    // to new Load's chain
+    for (unsigned i = 0; i < NumElems; ++i)
+      if (LoadMask[i]) {
+        SDValue Elt = peekThroughBitcasts(Elts[i]);
+        LoadSDNode *Ld = cast<LoadSDNode>(Elt);
+        DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
                                       SDValue(NewLd.getNode(), 1));
       }
-
     return NewLd;
   };
 
   // LOAD - all consecutive load/undefs (must start/end with a load).
   // If we have found an entire vector of loads and undefs, then return a large
   // load of the entire vector width starting at the base pointer.
   // If the vector contains zeros, then attempt to shuffle those elements.
   if (FirstLoadedElt == 0 && LastLoadedElt == (int)(NumElems - 1) &&
if (TLI.isTypeLegal(VecVT)) {
       SDValue Ops[] = { LDBase->getChain(), LDBase->getBasePtr() };
       SDValue ResNode =
           DAG.getMemIntrinsicNode(X86ISD::VZEXT_LOAD, DL, Tys, Ops, VecSVT,
                                   LDBase->getPointerInfo(),
                                   LDBase->getAlignment(),
                                   false/*isVolatile*/, true/*ReadMem*/,
                                   false/*WriteMem*/);
 
-      // Make sure the newly-created LOAD is in the same position as LDBase in
-      // terms of dependency. We create a TokenFactor for LDBase and ResNode,
-      // and update uses of LDBase's output chain to use the TokenFactor.
-      if (LDBase->hasAnyUseOfValue(1)) {
-        SDValue NewChain =
-            DAG.getNode(ISD::TokenFactor, DL, MVT::Other, SDValue(LDBase, 1),
-                        SDValue(ResNode.getNode(), 1));
-        DAG.ReplaceAllUsesOfValueWith(SDValue(LDBase, 1), NewChain);
-        DAG.UpdateNodeOperands(NewChain.getNode(), SDValue(LDBase, 1),
+      // all references to chain for each component load should now
+      // point to new Load's chain
+      for (unsigned i = 0; i < NumElems; ++i)
+        if (LoadMask[i]) {
+          SDValue Elt = peekThroughBitcasts(Elts[i]);
+          LoadSDNode *Ld = cast<LoadSDNode>(Elt);
+          DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
                                         SDValue(ResNode.getNode(), 1));
         }
-
       return DAG.getBitcast(VT, ResNode);
     }
   }
 
   // VZEXT_MOVL - consecutive 32-bit load/undefs followed by zeros/undefs.
   if (IsConsecutiveLoad && FirstLoadedElt == 0 && LoadSize == 32 &&
       ((VT.is128BitVector() || VT.is256BitVector() || VT.is512BitVector()))) {
     MVT VecSVT = VT.isFloatingPoint() ? MVT::f32 : MVT::i32;
if (LoadSDNode *Ld = dyn_cast<LoadSDNode>(V1.getOperand(0))) {
           DAG.getMemIntrinsicNode(X86ISD::VZEXT_LOAD, dl, Tys, Ops,
                                   Ld->getMemoryVT(),
                                   Ld->getPointerInfo(),
                                   Ld->getAlignment(),
                                   false/*isVolatile*/, true/*ReadMem*/,
                                   false/*WriteMem*/);
 
         // Make sure the newly-created LOAD is in the same position as Ld in
-        // terms of dependency. We create a TokenFactor for Ld and ResNode,
-        // and update uses of Ld's output chain to use the TokenFactor.
-        if (Ld->hasAnyUseOfValue(1)) {
-          SDValue NewChain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
-                             SDValue(Ld, 1), SDValue(ResNode.getNode(), 1));
-          DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1), NewChain);
-          DAG.UpdateNodeOperands(NewChain.getNode(), SDValue(Ld, 1),
+        // terms of chain dependency.
+        DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
                                       SDValue(ResNode.getNode(), 1));
-        }
-
         return DAG.getBitcast(VT, ResNode);
       }
     }
 
     // Emit a zeroed vector and insert the desired subvector on its
     // first half.
     SDValue Zeros = getZeroVector(VT, Subtarget, DAG, dl);
     SDValue InsV = insert128BitVector(Zeros, V1.getOperand(0), 0, DAG, dl);