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

[Nirav Dave via Phabricator via llvm-commits]

niravd added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3809
+                                    User->getValueType(0), And, OtherOp);
+      DAG.ReplaceAllUsesWith(User, NewUser.getNode());
+      DAG.RemoveDeadNode(User);
----------------
samparker wrote:
> niravd wrote:
> > The call to CombineTo after this should update the users so you should do an update. IIUC the only reason for this is to commute the operands in some cases. I suspect this isn't necessary or should be done by applying the check to both operands.
> > 
> > 
> The issue here was that I need to replace one of Users operands with an AND that masks part of one of User's original operands. I originally tried to create the AND and then use ReplaceAllUsesWith on the AND, in the hope that it would update User. But then because the AND is using the value that I tried to replace, a cyclic dependency is introduced. So the only way I could see is by creating a completely new node. I then call ReplaceAllUsesWith so that I can delete the original User, thus allowing the load to be combined with the new AND node.
Ah! Take a look at UpdateNodeOperands. It should let you undo the construction of a cyclic dependency. 


================
Comment at: test/CodeGen/ARM/and-load-combine.ll:1
+; RUN: llc -mtriple=armv7 %s -o - | FileCheck %s
+; RUN: llc -mtriple=armv7eb %s -o - | FileCheck %s
----------------
samparker wrote:
> niravd wrote:
> > Please commit the test cases separately so we can see the differences more easily in phabricator.
> Sorry, I'm confused as to why that would make it easier?
It's because the CHECK-NOT: lsl lines are not intuitively obvious as to why they are there (I'm still not sure why those were selected, but presumably it's important). IIUC all that matters is that originally the larger loads are being used. That's very easy to see with with a prepatch test case checking for the original code generated. I suppose additional CHECK-NOTs would work as well, but I'm not sure if that confuses the CHECK-NOT: lsl 



https://reviews.llvm.org/D39604