This is an archive of the discontinued LLVM Phabricator instance.

[LV] NFC: Return both fixed and scalable Max VF from computeMaxVF.
ClosedPublic

Authored by sdesmalen on Mar 16 2021, 9:26 AM.

Details

Summary

This patch introduces a new class, MaxVFCandidates, that holds the
maximum vectorization factors that have been computed for both scalable
and fixed-width vectors.

This patch is NFC and just propagates the information. A follow-up patch
will use this information to create new VPlans.

Diff Detail

Event Timeline

sdesmalen created this revision.Mar 16 2021, 9:26 AM
sdesmalen requested review of this revision.Mar 16 2021, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 9:26 AM
Matt added a subscriber: Matt.Apr 17 2021, 9:06 AM
sdesmalen updated this revision to Diff 343185.May 5 2021, 1:59 PM

Rebased patch.

This revision is now accepted and ready to land.May 10 2021, 3:38 AM
fhahn added inline comments.May 10 2021, 4:02 AM
llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
208

I’’m not sure what the candidates refers to in the name and I couldn’t spot any reference in any of the comments. AFAICT it’s just a pair of VFs, so perhaps there’s no need to make special reference to the fact that it is used for the maximum VFs?

221

Would be good to address?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1232–1235

Comment needs updating

1631–1637

Comment needs updating

5768

Do we need the separate variables or could we just use the VF pair and adjust the scalable/fixed VFs as we go along?

5855

I guess this would pessimize the case where fixed width vectorisation would not require a scalar tail, if a scalable VF is present? Might be good to have a comment/TODO? Also might be good to move the comment that this checks if a tail is required up as well

sdesmalen updated this revision to Diff 344076.May 10 2021, 8:51 AM
sdesmalen marked 4 inline comments as done.

Addressed review comments.

Thanks for the review/comments!

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
208

Yes I wasn't too clear on what name to give this. I've renamed it to FixedScalableVFPair, but feel free to suggest an alternative if you know a better one!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5855

Yes, that's correct, that's something to fix in a follow-up patch. I actually have a draft patch that addresses this, but I didn't want to complicate this patch any further. I've added a FIXME as you suggested.

@fhahn are you happy with the rename? I've addressed all other comments as well.

fhahn added inline comments.May 17 2021, 1:12 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1233

This does not return none any more? It returns a pair with both set to 0. Perhaps we can just say Returns an upper bound for the vectorization factors (both fixed and scalable). If both factors are 0, vectorization and interleaving should be avoided up front.

5855

Sounds good. Would it be possible to add a test case for that scenario?

sdesmalen updated this revision to Diff 345911.May 17 2021, 9:36 AM
sdesmalen marked 2 inline comments as done.
  • Added test for trying to eliminate vector tail when scalable vectors are considered.
  • Updated comment.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5855

Good suggestion, I've added a test that is XFAIL'ed for now, so that we can re-enable it when the issue is fixed properly.

fhahn accepted this revision.May 17 2021, 3:14 PM

LGTM, thanks!

(The NFC part should probably be dropped from the title, as it changes behaviour as per fixme?)

(The NFC part should probably be dropped from the title, as it changes behaviour as per fixme?)

Good point, I'll update the commit message before I land it. Thanks for reviewing the patch!

This revision was landed with ongoing or failed builds.May 18 2021, 12:04 AM
This revision was automatically updated to reflect the committed changes.