[PATCH] D16611: Fix CombineToPreIndexedLoadStore O(n^2) behavior

[Tim Shen via llvm-commits]

timshen marked 3 inline comments as done.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:9596
@@ -9595,1 +9595,3 @@
 
+  // Caches for hasPredecessorHelper
+  SmallPtrSet<const SDNode *, 32> Visited;
----------------
iteratee wrote:
> Can you add a const annotation to N so that this is obviously correct?
I agree with you in technical perspective, but it's conventional not to make a function parameter passed by const value for reasons I don't quite know :P.

Another solution is to make a local const copy of N and use that, but then it's confusing in another way, since there are more variables.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6937
@@ -6936,3 +6936,3 @@
 
 bool
 SDNode::hasPredecessorHelper(const SDNode *N,
----------------
timshen wrote:
> iteratee wrote:
> > Can you add some header comments for hasPredecessorHelper while you're here?
> I agree with you in technical perspective, but it's conventional not to make a function parameter passed by const value for reasons I don't quite know :P.
> 
> Another solution is to make a local const copy of N and use that, but then it's confusing in another way, since there are more variables.
Sorry - above is the reply for the previous comment.

For this comment - I think we already got complete comment in include/llvm/CodeGen/SelectionDAGNodes.h


http://reviews.llvm.org/D16611