This is an archive of the discontinued LLVM Phabricator instance.

[SCEVExpander] Increase "cheap" expansion budget for loop invariants, but not loop exit values
Changes PlannedPublic

Authored by lebedev.ri on Dec 12 2022, 9:53 AM.

Details

Summary

It is known that having some magical cut-off does just that,
it prevents very large regressions, but is potentially suboptimal
in certain cases.

There has been several reports that the current budget
is just a tad bit too small, and by making SCEV smarter,
e.g. teaching it to handle new IR patterns,
instead of representing them as SCEVUnknown,
makes problem worse.

This current constant, 4, was never meant
to be The One True Value, but rather it seemed like
the right choice at the time.

Currently, we use it for 3 purposes:

  • loop exit value rewriting
  • loop trip/exit count expansion
  • loop invariant expansion

Now, the loop trip/exit count is a special case of
the loop invariant, so let's treat them as one.
Out of the three, loop exit value is the problematic one.

Let's split the budget into two, and bump the loop invariant one.

Diff Detail

Event Timeline

lebedev.ri created this revision.Dec 12 2022, 9:53 AM
lebedev.ri requested review of this revision.Dec 12 2022, 9:53 AM
lebedev.ri edited the summary of this revision. (Show Details)Dec 12 2022, 9:56 AM
nikic added a comment.Dec 12 2022, 1:53 PM

There has been several reports that the current budget is just a tad bit too small

At the same time, we also regularly get complaints about exit values getting expanded where just reusing the final IV value would be cheaper, and raising the expansion limit is certainly going to make that situation worse.

Raising the expansion budget will improve some cases and regress others. For a heuristic cutoff, this is expected and fine. However, I would expect some kind of supporting data that the new value is indeed, on average, an improvement and not a regression.

lebedev.ri retitled this revision from [SCEVExpander] Increase "cheap" expansion budget to [SCEVExpander] Increase "cheap" expansion budget for loop invariants, but not loop exit values.
lebedev.ri edited the summary of this revision. (Show Details)

Currently, we use the budget for 3 purposes:

  • loop exit value rewriting
  • loop trip/exit count expansion
  • loop invariant expansion

Now, the loop trip/exit count is a special case of
the loop invariant, so let's treat them as one.
Out of the three, loop exit value is indeed the problematic one.

Let's split the budget into two, and bump the loop invariant one.

mkazantsev added inline comments.Dec 14 2022, 2:18 AM
llvm/test/Transforms/IndVarSimplify/X86/eliminate-trunc.ll
10

NFC-regenerate tests? Yes, it' extremely annoying since they've updated this script. :(

lebedev.ri marked an inline comment as done.Dec 14 2022, 6:45 AM

With unchanged budget for loop exit value expansion, does anyone have any further comments here?
I'd like to add, it is unusual that unroll checks the cost of the trip count, clearly other passes don't?

nikic added a comment.Dec 16 2022, 6:17 AM

I'd be more comfortable with this if it adjusted just the unroll limit, where a higher cost is probably justifiable relative to the cost of everything else. This change affects a lot of transforms, and from the test diffs alone, some of them look non-profitable to me, e.g. most of the changes in llvm/test/Transforms/IndVarSimplify/post-inc-range.ll due to questionable LFTR transforms.

I'd be more comfortable with this if it adjusted just the unroll limit, where a higher cost is probably justifiable relative to the cost of everything else.

This change affects a lot of transforms, and from the test diffs alone, some of them look non-profitable to me, e.g. most of the changes in llvm/test/Transforms/IndVarSimplify/post-inc-range.ll due to questionable LFTR transforms.

I'm not sure i understand. This kind of change we see in post-inc-range.ll, is exactly why i bothered to unbreak the original isHighCostExpansion originally in the first place.

nikic added a comment.Dec 16 2022, 7:01 AM

I'd be more comfortable with this if it adjusted just the unroll limit, where a higher cost is probably justifiable relative to the cost of everything else.
This change affects a lot of transforms, and from the test diffs alone, some of them look non-profitable to me, e.g. most of the changes in llvm/test/Transforms/IndVarSimplify/post-inc-range.ll due to questionable LFTR transforms.

I'm not sure i understand. This kind of change we see in post-inc-range.ll, is exactly why i bothered to unbreak the original isHighCostExpansion originally in the first place.

I'm probably missing some subtlety of IV canonicalization here. As far as I can tell those transforms a) add more instructions outside the loop, b) add more instructions inside the loop and c) introduce a trunc inside the loop, which is an analysis blocker (ext is better than trunc) and all that to convert an slt into ne for an IV we can already analyze as-is. If there's a benefit to all this, it's not obvious to me.

nikic added a comment.Dec 27 2022, 6:14 AM

I don't think my previous comment on non-profitable transforms in post-inc-range.ll has been addressed.