[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:3809
+                                    User->getValueType(0), And, OtherOp);
+      DAG.ReplaceAllUsesWith(User, NewUser.getNode());
+      DAG.RemoveDeadNode(User);
----------------
niravd wrote:
> 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. 
Great, thanks.


================
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
----------------
spatel wrote:
> niravd wrote:
> > 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 
> > 
> Just a drive-by comment about codegen checks: there's a script to auto-generate the FileCheck lines at llvm/utils/update_llc_test_checks.py (and it has support for ARM triples, but not all of the ones in this test file - any help in getting more triples to work with the script is greatly appreciated!).
> 
> If you use that, then it becomes very simple to add tests with complete checking *before* you make a functional change that will change the asm. This makes it easier to review exactly what changes. It also avoids the inherent fragility of 'CHECK-NOT' (imagine that someone has an 'lsl' string in the path to this test file and FileCheck sees that path string).
Ok, thank you both. I'll make the changes and try the script, hopefully with a patch for it as well.


https://reviews.llvm.org/D39604