[PATCH] D62633: Recommit r360171: [DAGCombiner] Avoid creating large tokenfactors in visitTokenFactor.

Thu May 30 15:00:06 PDT 2019

fhahn added a comment.

In D62633#1522935 <https://reviews.llvm.org/D62633#1522935>, @niravd wrote:

> In D62633#1522420 <https://reviews.llvm.org/D62633#1522420>, @fhahn wrote:
>
> > In D62633#1522239 <https://reviews.llvm.org/D62633#1522239>, @niravd wrote:
> >
> > > Can you get away with just adding all remaining TFs (not their operands) to Ops? That way we keep the TF smaller and the pruning search still happens. That seems sufficient to fix the dropped operator issue.
> >
> >
> > Yes we can, kind of. If we just add all remaining TFs, we might get stuck in a loop, because in some cases the simplified TokenFactor == N and N gets added to the worklist, because it is the single user of another tokenfactor. I changed it to expand the first outstanding tokenfactor and add the remaining ones unexpanded. That way, we should not get stuck in a loop, while keeping the size small.
> >
> > What do you think?
>
>
> The problem with doing an small change is that we will definitely revisit the node again because it's changed and will then are likely to do another minor change inlining the next possible token factor.  I suspect we're better off deferring updating the Changed predicate for TF operands until we've actually inlined its operands.

Agreed. I went for the current version to avoid a combine cycle which seemed to be caused by the manual AddToWorklist call earlier on. I'll dig some more.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62633