This is an archive of the discontinued LLVM Phabricator instance.

[LV] Vectorize without versioning-for-unit-stride under -Os/-Oz
ClosedPublic

Authored by Ayal on Jun 7 2020, 9:05 AM.

Details

Summary

If a loop is in a function marked OptSize, Loop Access Analysis should refrain from generating runtime checks for unit strides that will version the loop.

If a loop is in a function marked OptSize and its vectorization is enabled, it should be vectorized w/o any versioning.

Fixes PR46228.

Diff Detail

Event Timeline

Ayal created this revision.Jun 7 2020, 9:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2020, 9:06 AM
SjoerdMeijer accepted this revision.Jun 9 2020, 3:25 AM

Thanks for fixing. This looks like a good and straightforward fix to me.

This revision is now accepted and ready to land.Jun 9 2020, 3:25 AM
fhahn added inline comments.Jun 9 2020, 4:28 AM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
1838

I think it might be slightly preferable to let LV drive the decision whether to version or not based on cost estimates (and LAA is used by other passes as well, which might have different requirements).

Did you consider disabling generating the codes for (I think it should happen emitSCEVChecks) conditionally on optsize? IIUC we should only need to generate code for predicates, if either we need runtime checks or have symbolic strides. And runtime checks are already rejected in runtimeChecksRequired.

Ayal marked an inline comment as done.Jun 13 2020, 10:59 PM
Ayal added inline comments.
llvm/lib/Analysis/LoopAccessAnalysis.cpp
1838

Would it be better if LV passes a "only one copy of the loop is allowed" flag to analyzeLoop(), via the constructor of LAI, instead of the latter checking for OptSize? This requirement of OptSize is common to all other passes, right?

Suppressing emitSCEVChecks would appease the assert, but LAI should refrain from collecting Strided Accesses in order (for getPtrStride()) to consider related accesses as non-unit strided accesses. It already does so under a cl::opt flag, for all its users.

bmahjour removed a subscriber: bmahjour.Jun 15 2020, 6:38 AM
fhahn added inline comments.Jun 24 2020, 2:36 PM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
1838

Would it be better if LV passes a "only one copy of the loop is allowed" flag to analyzeLoop(), via the constructor of LAI, instead of the latter checking for OptSize? This requirement of OptSize is common to all other passes, right?

(Sorry for the long delay!)

I think something like that would be preferable, as it makes explicit the interaction between LV/LAI. But I am not sure if that is possible at the moment though, because we get LAI as analysis.

I am not sure if there's a feasible alternative without too much refactoring and it is probably not worth blocking the change on that, especially given that there's already precedence for using opt flags in a similar way.

This revision was automatically updated to reflect the committed changes.
Ayal marked an inline comment as done.Jul 7 2020, 5:10 AM
Ayal added inline comments.
llvm/lib/Analysis/LoopAccessAnalysis.cpp
1838

OK, thanks, unblocked the change :-)

uabelho added a subscriber: uabelho.Jul 9 2020, 1:02 AM

Hi,

I start seeing a crash with this patch:
https://bugs.llvm.org/show_bug.cgi?id=46652