[DAG] add helper to bind memop chains; NFCI

[spatel]

Summary

This step is just intended to reduce code duplication rather than change any functionality.

However, I noticed that PowerPC has PPCTargetLowering::spliceIntoChain(), and it seems to do almost the same thing as the x86 code. My understanding of this still isn't good, so it's not clear to me if the x86 code is overkill or if the PPC version is not going far enough. At the least, the code comment for spliceIntoChain() doesn't match the code itself: we don't actually add the old chain to the TokenFactor there?

Pasting the PPC code here for reference:

// Given the head of the old chain, ResChain, insert a token factor containing
// it and NewResChain, and make users of ResChain now be users of that token
// factor.
void PPCTargetLowering::spliceIntoChain(SDValue ResChain,
                                        SDValue NewResChain,
                                        SelectionDAG &DAG) const {
  if (!ResChain)
    return;

  SDLoc dl(NewResChain);

  SDValue TF = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
                           NewResChain, DAG.getUNDEF(MVT::Other));
  assert(TF.getNode() != NewResChain.getNode() &&
         "A new TF really is required here");

  DAG.ReplaceAllUsesOfValueWith(ResChain, TF);
  DAG.UpdateNodeOperands(TF.getNode(), ResChain, NewResChain);
}

[niravd]

Nice cleanup! LGTM modulo nits.

From my reading of the PPC backend, the PowerPC version is that the generic version is marginally improving debugability and there is one path in PPC target where the OldChain is not set and needs to be checked. Since the early return is semantically equivalent, I see no reason not to just add the initial check and remove the PPC splice operation.

include/llvm/CodeGen/SelectionDAG.h

1179

It'd be nice if the name bindChain could probably be clearer. Maybe something along the lines of "markSymettricMemoryOrderings".

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

7243

Nit: use getChain() for these two.

[spatel]

include/llvm/CodeGen/SelectionDAG.h

1179

Does "makeEquivalentMemoryOrdering" capture the intent better?

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

7243

IIUC, getChain() gives us the chain operand rather than the chain result. When I try this, we get many test failures because, for example, the chain operand may not have exactly the 2 operands that UpdateNodeOperands() is expecting for a LoadSDNode.

[spatel]

Patch updated:

1. Change function name to "makeEquivalentMemoryOrdering()" to make intent clearer.
2. Change 2nd param name and comments from "NewLoad" to "NewMemOp" (it is not necessarily a load).
3. Add TODO comment for related PPC function.

[spatel]

Ping.

The patch was approved with the requirement to use "getChain()", but IIUC, we can't do that here. Can someone confirm if I understood that correctly?

[niravd]

include/llvm/CodeGen/SelectionDAG.h

1179

SGTM.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

7243

Right, of course. Nevermind.

include/llvm/CodeGen/SelectionDAG.h

#endif
   void ReplaceAllUsesOfValueWith(SDValue From, SDValue To);
 
   /// Like ReplaceAllUsesOfValueWith, but for multiple values at once.
   /// This correctly handles the case where
   /// there is an overlap between the From values and the To values.
   void ReplaceAllUsesOfValuesWith(const SDValue *From, const SDValue *To,
                                   unsigned Num);
 
+  /// If an existing load has uses of its chain, create a token factor node with
+  /// that chain and the new memory node's chain and update users of the old
+  /// chain to the token factor. This ensures that the new memory node will have
+  /// the same relative memory dependency position as the old load.
+  void bindChains(LoadSDNode *Old, SDValue New);
+
   /// Topological-sort the AllNodes list and a
   /// assign a unique node id for each node in the DAG based on their
   /// topological order. Returns the number of nodes.
   unsigned AssignTopologicalOrder();
 
   /// Move node N in the AllNodes list to be immediately
   /// before the given iterator Position. This may be used to update the
   /// topological ordering when the list of nodes is modified.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

static SDValue narrowExtractedVectorLoad(SDNode *Extract, SelectionDAG &DAG) {
   unsigned Offset = ExtIdx->getZExtValue() * VT.getScalarType().getStoreSize();
 
   // TODO: Use "BaseIndexOffset" to make this more effective.
   SDValue NewAddr = DAG.getMemBasePlusOffset(BaseAddr, Offset, DL);
   MachineFunction &MF = DAG.getMachineFunction();
   MachineMemOperand *MMO = MF.getMachineMemOperand(Ld->getMemOperand(), Offset,
                                                    VT.getStoreSize());
   SDValue NewLd = DAG.getLoad(VT, DL, Ld->getChain(), NewAddr, MMO);
-
-  // The new load must have the same position as the old load in terms of memory
-  // dependency. Create a TokenFactor for Ld and NewLd and update uses of Ld's
-  // output chain to use that TokenFactor.
-  // TODO: This code is based on a similar sequence in x86 lowering. It should
-  // be moved to a helper function, so it can be shared and reused.
-  if (Ld->hasAnyUseOfValue(1)) {
-    SDValue OldChain = SDValue(Ld, 1);
-    SDValue NewChain = SDValue(NewLd.getNode(), 1);
-    SDValue TokenFactor = DAG.getNode(ISD::TokenFactor, DL, MVT::Other,
-                                      OldChain, NewChain);
-    DAG.ReplaceAllUsesOfValueWith(OldChain, TokenFactor);
-    DAG.UpdateNodeOperands(TokenFactor.getNode(), OldChain, NewChain);
-  }
-
+  DAG.bindChains(Ld, NewLd);
   return NewLd;
 }
 
 SDValue DAGCombiner::visitEXTRACT_SUBVECTOR(SDNode* N) {
   EVT NVT = N->getValueType(0);
   SDValue V = N->getOperand(0);
 
   // Extract from UNDEF is UNDEF.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

if (Dbg->getKind() == SDDbgValue::SDNODE &&
       ClonedDVs.push_back(Clone);
       Dbg->setIsInvalidated();
     }
   }
   for (SDDbgValue *I : ClonedDVs)
     AddDbgValue(I, ToNode, false);
 }
 
+void SelectionDAG::bindChains(LoadSDNode *OldLoad, SDValue NewLoadVal) {
+  assert(isa<MemSDNode>(NewLoadVal.getNode()) && "Expected a memop node");
+  if (!OldLoad->hasAnyUseOfValue(1))
+    return;
+
+  // The new load must have the same position as the old load in terms of memory
+  // dependency. Create a TokenFactor for the old load and new load and update
+  // uses of the old load's output chain to use that TokenFactor.
+  SDValue OldChain = SDValue(OldLoad, 1);
+  SDValue NewChain = SDValue(NewLoadVal.getNode(), 1);
+  SDValue TokenFactor =
+      getNode(ISD::TokenFactor, SDLoc(OldLoad), MVT::Other, OldChain, NewChain);
+  ReplaceAllUsesOfValueWith(OldChain, TokenFactor);
+  UpdateNodeOperands(TokenFactor.getNode(), OldChain, NewChain);
+}
+
 //===----------------------------------------------------------------------===//
 //                              SDNode Class
 //===----------------------------------------------------------------------===//
 
 bool llvm::isNullConstant(SDValue V) {
   ConstantSDNode *Const = dyn_cast<ConstantSDNode>(V);
   return Const != nullptr && Const->isNullValue();
 }

lib/Target/X86/X86ISelLowering.cpp

static SDValue EltsFromConsecutiveLoads(EVT VT, ArrayRef<SDValue> Elts,
 
   auto CreateLoad = [&DAG, &DL](EVT VT, LoadSDNode *LDBase) {
     auto MMOFlags = LDBase->getMemOperand()->getFlags();
     assert(!(MMOFlags & MachineMemOperand::MOVolatile) &&
            "Cannot merge volatile loads.");
     SDValue NewLd =
         DAG.getLoad(VT, DL, LDBase->getChain(), LDBase->getBasePtr(),
                     LDBase->getPointerInfo(), LDBase->getAlignment(), MMOFlags);
-
-    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),
-                             SDValue(NewLd.getNode(), 1));
-    }
-
+    DAG.bindChains(LDBase, NewLd);
     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)) {
       SDVTList Tys = DAG.getVTList(VecVT, MVT::Other);
       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),
-                               SDValue(ResNode.getNode(), 1));
-      }
-
+      DAG.bindChains(LDBase, ResNode);
       return DAG.getBitcast(VT, ResNode);
     }
   }
 
   return SDValue();
 }
 
 static Constant *getConstantVector(MVT VT, const APInt &SplatValue,
if (V.getOpcode() == ISD::BUILD_VECTOR ||
     LoadSDNode *Ld = cast<LoadSDNode>(BC);
     SDValue BaseAddr = Ld->getOperand(1);
     EVT SVT = BroadcastVT.getScalarType();
     unsigned Offset = BroadcastIdx * SVT.getStoreSize();
     SDValue NewAddr = DAG.getMemBasePlusOffset(BaseAddr, Offset, DL);
     V = DAG.getLoad(SVT, DL, Ld->getChain(), NewAddr,
                     DAG.getMachineFunction().getMachineMemOperand(
                         Ld->getMemOperand(), Offset, SVT.getStoreSize()));
-
-    // Make sure the newly-created LOAD is in the same position as Ld in
-    // terms of dependency. We create a TokenFactor for Ld and V,
-    // 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(V.getNode(), 1));
-      DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1), NewChain);
-      DAG.UpdateNodeOperands(NewChain.getNode(), SDValue(Ld, 1),
-                             SDValue(V.getNode(), 1));
-    }
+    DAG.bindChains(Ld, V);
   } else if (!BroadcastFromReg) {
     // We can't broadcast from a vector register.
     return SDValue();
   } else if (BroadcastIdx != 0) {
     // We can only broadcast from the zero-element of a vector register,
     // but it can be advantageous to broadcast from the zero-element of a
     // subvector.
     if (!VT.is256BitVector() && !VT.is512BitVector())