[PATCH] D39604: [DAGCombine] Move AND nodes to multiple load leaves

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3755
+      // Allow one node which will masked along with any loads found.
+      if (NodeToMask)
+        return false;
----------------
niravd wrote:
> samparker wrote:
> > niravd wrote:
> > > If  you pull out the the checking and recording of NodeToMask to after the switch and return immediately on sucessful cases, you can out the default case and allows us to succeed with an "and of load" if one of the loads is not a valid extLoad. 
> > I'm not sure that works here, as I want to check all the operands and an early exit on the successful case could miss other valid nodes.
> Sorry, I miswrote. You should "continue" to the next iteration of the loop on success.
Ah, ok, I'll give that a go.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3805
+                            SDValue(N, 0), Mask);
+  SDValue N0 = SDValue(N, 0);
+  for (auto *User : N->uses()) {
----------------
niravd wrote:
> samparker wrote:
> > niravd wrote:
> > > You should be able to replace most of this by using ReplaceAllUsesWith and then using UpdateNodeOperands to fix up the operands of the And Node. i.e.:
> > > 
> > > DAG.ReplaceAllUsesWith(N, And);
> > > DAG.UpdateNodeOperands(And, SDValue(N,0), Mask));
> > > 
> > > It's probably worth inlining this function as well. 
> > > 
> > But as 'And' uses 'N', won't this cause the cyclic dependency that I'm trying to avoid? Or is there a nice way of initialising a dummy 'And' node that doesn't use N?
> It will but it won't loop forever and you'll immediately fix it. See the implementation of SelectionDAG::makeEquivalentMemoryOrdering. 
Great, thanks.


https://reviews.llvm.org/D39604