[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:3739
 
+bool DAGCombiner::SearchForNarrowLoads(SDNode *N,
+                                       SmallPtrSetImpl<LoadSDNode*> &Loads,
----------------
niravd wrote:
> IIUC if you see a non-handled case (AND, OR, XOR, LOAD) you will give up because we cannot fully remove the AND. 
> 
> What about partial success? Consider: AND (OR (LOAD x) (ADD y z)) MASK -> OR (ZEXTLOAD x) (AND (ADD y z) MASK). As long as we only generate 1 or fewer ANDs its a net win*. I also suspect that narrowing loads regardless would be a net-positive for SimplifyDemandedBits, though other reviews would know more about it than I do. 
> 
> 
> 
> 
Ok, thanks, I'll look into it.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3809
+                                    User->getValueType(0), And, OtherOp);
+      DAG.ReplaceAllUsesWith(User, NewUser.getNode());
+      DAG.RemoveDeadNode(User);
----------------
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.


================
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
----------------
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?


https://reviews.llvm.org/D39604