This is an archive of the discontinued LLVM Phabricator instance.

Compute estimated trip counts for multiple exit loops
ClosedPublic

Authored by reames on Dec 8 2021, 10:30 AM.

Details

Summary

This change allows us to estimate trip count from profile metadata for all multiple exit loops. We still do the estimate only from the latch, but that's fine as it causes us to over estimate the trip count at worst.

Reviewing the uses of the API, all but one are cases where we restrict a loop transformation (unroll, and vectorize respectively) when we know the trip count is short enough. So, as a result, the change makes these passes strictly less aggressive. The test change illustrates a case where we'd previously have runtime unrolled a loop which ran fewer iterations than the unroll factor. This is definitely unprofitable.

The one case where an upper bound on estimate trip count could drive a more aggressive transform is peeling, and I duplicated the logic being removed from the generic estimation there to keep it the same. The resulting heuristic makes no sense and should probably be immediately removed, but we can do that in a separate change.

This was noticed when analyzing regressions on D113939.

I plan to come back and incorporate estimated trip counts from other exits, but that's a minor improvement which can follow separately.

Diff Detail

Event Timeline

reames created this revision.Dec 8 2021, 10:30 AM
reames requested review of this revision.Dec 8 2021, 10:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 10:30 AM
anna added a comment.Dec 8 2021, 1:10 PM

Nice find! This is obviously a good thing to do for unrolling. Any ideas why there's no vectorization test cases to update since we do support multi-exit vectorization?

reames added a comment.Dec 8 2021, 1:36 PM

Nice find! This is obviously a good thing to do for unrolling. Any ideas why there's no vectorization test cases to update since we do support multi-exit vectorization?

Well, purely because I didn't write one. The unrolling test change didn't exist either before I wrote the one showing a diff this morning. Note that even that one requires a non-default flag value to show up.

I don't think the change in vectorization is likely to be interesting, and crafting a test will be tedious. Do you want me to bother?

anna accepted this revision.Dec 9 2021, 6:35 AM

LGTM.

Nice find! This is obviously a good thing to do for unrolling. Any ideas why there's no vectorization test cases to update since we do support multi-exit vectorization?

Well, purely because I didn't write one. The unrolling test change didn't exist either before I wrote the one showing a diff this morning. Note that even that one requires a non-default flag value to show up.

I don't think the change in vectorization is likely to be interesting, and crafting a test will be tedious. Do you want me to bother?

That's fine. Was just curious on how this would show up with vectorization. Given it is tedious and non-obvious, the patch as-is looks good.

This revision is now accepted and ready to land.Dec 9 2021, 6:35 AM
This revision was landed with ongoing or failed builds.Dec 9 2021, 9:54 AM
This revision was automatically updated to reflect the committed changes.