This is an archive of the discontinued LLVM Phabricator instance.

[LV] Move runtime pointer size check to LVP::plan().
ClosedPublic

Authored by fhahn on Mar 15 2021, 8:43 AM.

Details

Summary

This removes the need for the remaining doesNotMeet check and instead
directly checks if there are too many runtime checks for vectorization
in the planner.

A subsequent patch will adjust the logic used to decide whether to
vectorize with runtime to consider their cost more accurately.

Diff Detail

Event Timeline

fhahn created this revision.Mar 15 2021, 8:43 AM
fhahn requested review of this revision.Mar 15 2021, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 8:43 AM
bmahjour removed a subscriber: bmahjour.Mar 15 2021, 9:10 AM

So the basic idea here is that LVL should only care about legality, but it should be up to it's users to care about profitability?
What do you think about the other limit?

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
45–52

These should move, too.

fhahn updated this revision to Diff 332707.Mar 23 2021, 9:52 AM

Rebase & ping :)

So the basic idea here is that LVL should only care about legality, but it should be up to it's users to care about profitability?

Yes, LoopVectorLegality, as the name implies ideally should just focus on deciding whether it is legal to vectorize or not IMO.

LVL does not really have any capabilities to make real cost-based decisions, because it is mostly independent of concrete VFs.

The decision whether to vectorize with runtime checks was not really taken by LVL even before this change. The decision was taken late, using a helper defined as part of LVL.

What do you think about the other limit?

I think once we move to make decisions related to the other limits on a more cost-based basis we should move them as well. But we should move them once at a time, so we can adjust & react to potential fallout.

fhahn added inline comments.Mar 23 2021, 9:54 AM
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
45–52

Agreed, but I think it only really makes sense to move those once we make better cost-based decisions for them as well. WDYT?

lebedev.ri accepted this revision.Mar 23 2021, 2:24 PM

LGTM
It seems that all/most of the reviewers who are familiar with this aren't responsive..

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
45–52

Can you at least add a FIXME here?

This revision is now accepted and ready to land.Mar 23 2021, 2:24 PM
fhahn added inline comments.Mar 25 2021, 3:58 AM
llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
45–52

Will do!

fhahn updated this revision to Diff 333827.Mar 29 2021, 5:25 AM

Added TODO comment to LVL.cpp

LGTM
It seems that all/most of the reviewers who are familiar with this aren't responsive..

Thanks! I think it's been long enough, we can always adjust/revert if we missed anything serious.

Added TODO comment to LVL.cpp

LGTM
It seems that all/most of the reviewers who are familiar with this aren't responsive..

Thanks! I think it's been long enough, we can always adjust/revert if we missed anything serious.

SGTM, thank you!

This revision was landed with ongoing or failed builds.Mar 29 2021, 6:20 AM
This revision was automatically updated to reflect the committed changes.