This is an archive of the discontinued LLVM Phabricator instance.

[LSR] Complexity tradeoff in GenerateReassociationsImpl
AbandonedPublic

Authored by mkazantsev on Dec 1 2017, 5:49 AM.

Details

Summary

During its work, GenerateReassociationsImpl may end up trying calculating a sum
of a huge amount of huge SCEVs. Attempts to simplify them lead to huge compile
time problem. This patch applies a heuristic that limits simplification recursion depth
depending on number of arguments in this method to get an acceptible compile time
in corner cases while it should not affect most of real situations.

Diff Detail

Event Timeline

mkazantsev created this revision.Dec 1 2017, 5:49 AM
sanjoy requested changes to this revision.Dec 26 2017, 1:16 PM
sanjoy added inline comments.
lib/Transforms/Scalar/LoopStrengthReduce.cpp
3497

This worries me from a design perspective -- the Depth parameter definitely should not be part of the public API of SCEV (I had not noticed that we've added these parameters to the public API). We can't guarantee its meaning or even its presence to passes that use SCEV.

Can we instead make SCEV smarter to avoid spending a lot of cycles in pathological cases like this?

This revision now requires changes to proceed.Dec 26 2017, 1:16 PM

The other way is limiting SCEV transformation not by depth only, but also by the size of operands list, if it makes sense.

The other way is limiting SCEV transformation not by depth only, but also by the size of operands list, if it makes sense.

I think your current approach is fine (penalize depth for high "breadth"), but it just needs to somehow happen within SCEV (so that "depth" remains a SCEV implementation detail).

mkazantsev abandoned this revision.Feb 26 2018, 2:57 AM