[PATCH] D56719: [DAGCombine] Enable more pre-indexed stores

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I just did a very brief experiment, and just checking `if (Val == BasePtr || Val == Ptr || Ptr->isPredecessorOf(Val.getNode()))` seems to do the right thing without any additional checks.  Is all this extra code really necessary?



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12666
+    // - and Val is not in the use-def chain between BasePtr and the store,
+    //   this prevents Val from being dependent of the pre-inc'd address.
+    if (BasePtr.getNode()->isPredecessorOf(Val.getNode())) {
----------------
samparker wrote:
> efriedma wrote:
> > I think this could still use a bit more clarification...
> > 
> > There are two different uses of ReplaceAllUsesOfValueWith which could cause a cycle related to stored value: the replacement of Ptr with the pre-indexed result, and the rewritten arithmetic relative to BasePtr in OtherUses.  Are you trying to prevent both those cycles?  Could we be more aggressive if we separated those checks?
> So yes, I am trying to make sure that Ptr can be replaced legally and I have assumed that by allowing the last scheduled user to be a pre-indexed store, we don't have to worry about OtherUses. Though I am now confused after re-reading statement (3) from above which appears to directly contradict my thoughts.
There's a hasPredecessorHelper call later; I think that handles cases like (3).


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12669
+      for (auto *U : BasePtr.getNode()->uses()) {
+        if (Val.getNode() == U)
+          return false;
----------------
samparker wrote:
> efriedma wrote:
> > The equality check here seems suspect; what if Val is a use of one of the direct users of BasePtr?
> Hmm. Would it be valid to create an ADD/SUB node and check whether it becomes a predecessor of Val...?
ADD/SUB of what, exactly?

More generally, we want to avoid performing predecessor checking more than once, since it's expensive.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56719/new/

https://reviews.llvm.org/D56719