Page MenuHomePhabricator

[SCEV] Prohibit SCEV transformations for huge SCEVs

Authored by mkazantsev on Jul 28 2017, 5:11 AM.



Currently SCEV attempts to limit transformations so that they do not work with
big SCEVs (that may take almost infinite compile time). But for this, it uses heuristics
such as recursion depth and number of operands, which do not give us a guarantee
that we don't actually have big SCEVs. This situation is still possible, though it is not
likely to happen. However, the bug PR33494 showed a bunch of simple corner case
tests where we still produce huge SCEVs, even not reaching big recursion depth etc.

This patch introduces a concept of 'huge' SCEVs. A SCEV is huge if its expression
size (intoduced in D35989) exceeds some threshold value. We prohibit optimizing
transformations if any of SCEVs we are dealing with is huge. This gives us a reliable
check that we don't spend too much time working with them.

As the next step, we can possibly get rid of old limiting mechanisms, such as recursion
depth thresholds.

Diff Detail


Event Timeline

mkazantsev created this revision.Jul 28 2017, 5:11 AM

Reused smart version of any_of

mkazantsev planned changes to this revision.Oct 9 2017, 11:27 PM

Some tests from fail when huge expressions threshold is set to low values (like 100). It is not automatically this patch's problem, but this should be fixed before this is merged.

mkazantsev added a comment.EditedOct 9 2017, 11:53 PM

UPD: The mentioned crash is actually assertion failure on cast<SCEVAddRecExpr>. Something happens to be non-addrec after we limit transformations by huge scevs. This bug also should be (in theory) reproducible without this patch, but I don't have an example.

mkazantsev abandoned this revision.Feb 26 2018, 2:56 AM
mkazantsev reclaimed this revision.Jan 14 2019, 10:28 PM

We need it to fix PR40292. Reclaiming.

Updated test to preserve current behavior (it creates *really* big SCEVs in test_05).

reames requested changes to this revision.Jan 23 2019, 10:08 AM

You need some tests highlighting the limit working in practice. These can be easily written by using non-default threshold values.

858 ↗(On Diff #181726)

Please add a comment here explaining what "huge" means and is used for. This the obvious point for someone reading the code to try to find the explanation.

862 ↗(On Diff #181726)

Probably better to use an ArrayRef as the argument.

2909 ↗(On Diff #181726)

Not related to your review, but I notice inconsistency about whether we merge adjacent constants before or after the bailout check.

This revision now requires changes to proceed.Jan 23 2019, 10:08 AM
mkazantsev marked 2 inline comments as done.Jan 23 2019, 11:58 PM
mkazantsev added inline comments.
862 ↗(On Diff #181726)

SCEV uses SmallVectorImpl to pass argument lists in all places. What you propose is inconsistent with the current code. We can do a global refactoring if there is any benefit from it.

2909 ↗(On Diff #181726)

We can change that separately.

Added comments.

reames requested changes to this revision.Jan 24 2019, 8:16 AM

Still needs tests

This revision now requires changes to proceed.Jan 24 2019, 8:16 AM
sanjoy added inline comments.Jan 24 2019, 9:41 AM
862 ↗(On Diff #181726)

SCEV uses SmallVectorImpl in many places because it needs to modify the container in place (e.g. getAddExpr will reduce the size of SmallVectorImpl in place when folding constants). Functions that do not modify the input sequence in place should use ArrayRef.

mkazantsev marked an inline comment as done.Jan 29 2019, 1:41 AM
mkazantsev added inline comments.
862 ↗(On Diff #181726)

It makes sense, yet it was untrue for SCEV code. I've commited rL352466 to leverage this. Will rebase on top of it.

Added some tests to show that it works, rebased, replaced with ArrayRef.

reames accepted this revision.Jan 29 2019, 10:29 AM


This revision is now accepted and ready to land.Jan 29 2019, 10:29 AM
This revision was automatically updated to reflect the committed changes.