[PATCH] D33649: [DAG] add helper to bind memop chains; NFCI

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1179
+  /// the same relative memory dependency position as the old load.
+  void bindChains(LoadSDNode *Old, SDValue New);
+
----------------
niravd wrote:
> It'd be nice if the name bindChain could probably be clearer. Maybe something along the lines of  "markSymettricMemoryOrderings".
> 
Does "makeEquivalentMemoryOrdering" capture the intent better?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7243
+  // uses of the old load's output chain to use that TokenFactor.
+  SDValue OldChain = SDValue(OldLoad, 1);
+  SDValue NewChain = SDValue(NewLoadVal.getNode(), 1);
----------------
niravd wrote:
> Nit: use getChain() for these two.
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.


https://reviews.llvm.org/D33649