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

[Sam Parker via Phabricator via llvm-commits]

samparker marked 4 inline comments as done.
samparker added inline comments.


================
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,
----------------
efriedma wrote:
> 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?
Yes, scheduled last is what I mean.


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


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12669
+      for (auto *U : BasePtr.getNode()->uses()) {
+        if (Val.getNode() == U)
+          return false;
----------------
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...?


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

https://reviews.llvm.org/D56719