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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

hasPredecessorHelper supports a limit, that would be fine, probably.  (The check is probably fast enough in practice in common cases, so it should be fine as long as it doesn't explode quadratically in extreme cases.)



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12664
+    // be stored. But a cycle cannot occur if:
+    // - the pointer operand of the store is the last use of the base pointer.
+    // - and Val is not in the use-def chain between BasePtr and the store,
----------------
Could you clarify what "last use" means?  It's not obvious when you're dealing with a DAG.  I guess it means the store must be scheduled after every other use of BasePtr?


================
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())) {
----------------
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?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12669
+      for (auto *U : BasePtr.getNode()->uses()) {
+        if (Val.getNode() == U)
+          return false;
----------------
The equality check here seems suspect; what if Val is a use of one of the direct users of BasePtr?


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

https://reviews.llvm.org/D56719