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

[Nirav Dave via Phabricator via llvm-commits]

niravd added a comment.





================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3739
 
+bool DAGCombiner::SearchForNarrowLoads(SDNode *N,
+                                       SmallPtrSetImpl<LoadSDNode*> &Loads,
----------------
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. 






================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3758
+      if (isAndLoadExtLoad(Mask, Load, Load->getValueType(0), ExtVT,
+                           NarrowLoad)) {
+        if (!NarrowLoad)
----------------
if (isAndLoadExtLoad(Mask, Load, Load->getValueType(0), ExtVT, NarrowLoad) && NarrowLoad) {


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3773
+    case ISD::AND:
+      SearchForNarrowLoads(Op.getNode(), Loads, Mask);
+      break;
----------------
I believe you need a return here or something like (AND (AND (ADD x y) MASK1) (LOAD z) MASK2) will result in the ADD missing MASK2. 


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




================
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
----------------
Please commit the test cases separately so we can see the differences more easily in phabricator.


https://reviews.llvm.org/D39604