Page MenuHomePhabricator

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

Authored by fhahn on May 29 2019, 3:59 PM.

Details

Summary

If we hit the limit, we do expand the outstanding tokenfactors.
Otherwise, we might drop nodes with users in the unexpanded
tokenfactors. This fixes the crashes reported by Jordan Rupprecht.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.May 29 2019, 3:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2019, 3:59 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

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.

If my intuition is wrong and we do need to CompletelyExpanded you should disable the pruning search itself when it's not being used.

fhahn updated this revision to Diff 202109.May 29 2019, 8:10 PM

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?

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.

fhahn added a comment.May 30 2019, 2:59 PM

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.

fhahn updated this revision to Diff 202334.May 30 2019, 4:52 PM

Only add tokenfactors instead of inlining the first TF. Adjust logic for adding
TFs to the worklist accordingly.

fhahn updated this revision to Diff 202455.May 31 2019, 10:03 AM

Bump cutoff to 2048 again, instead of 5 used for testing.

fhahn updated this revision to Diff 202618.Jun 2 2019, 12:01 PM

Add option to adjust inline limit, for testing and added test that crashes with
the original patch.

niravd accepted this revision.Jun 2 2019, 3:24 PM

LGTM modulo typo nit. Thanks!

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1851 ↗(On Diff #202618)

nit: s/tokenfacto/Token Factor/g

This revision is now accepted and ready to land.Jun 2 2019, 3:24 PM
fhahn marked an inline comment as done.Jun 2 2019, 6:27 PM

Thanks!

This revision was automatically updated to reflect the committed changes.