[PATCH] D39604: [DAGCombine] Move AND nodes to multiple load leaves
Nirav Dave via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 27 10:53:04 PST 2017
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
More information about the llvm-commits
mailing list