[PATCH] D61397: [DAGCombiner] Avoid creating large tokenfactors in visitTokenFactor

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1767
 SDValue DAGCombiner::visitTokenFactor(SDNode *N) {
+  // Limit the number of nodes to explore, to avoid quadratic compile times.
+  // The limit helps to limit compile time for basic blocks with a large number
----------------
niravd wrote:
> I this that this is too restrictive. This check effectively curtails the second and third passes (The first being the quick two operand check). 
> 
> The third pass is a fairly expensive chain predecessor search to see if there are redundancies in the operands (if A and its chain predecessor B are both in the list, you can drop B) but already has a cutoff of 1024 steps. 
> 
> The second pass is a linear pass to weed out trivial redundancies in the list. Due to the way that chain replacements happen, it's extremely common for TokenFactors to have redundant operands in them and I worry that curtialing this will have nontrivial degradation for relatively little compile time improvement. 
> 
> If this is making a notable difference, I suspect we should  change the second pass to not inline a TokenFactor if the number of operands in the new TokenFactor would be too large.
> 
Thanks! I've limited the number of Ops to collect for inlining in the second pass. Is that along the lines you were thinking? I will also check if it helps to skip tokefactors that will get us to exceed a limit here.


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

https://reviews.llvm.org/D61397