Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
300 msx64 debian > Polly.Isl/CodeGen::multiple-codegens.ll
Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen -polly-codegen-verify -polly-scops -polly-opt-isl -polly-codegen -polly-scops -polly-codegen -S < /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/multiple-codegens.ll | FileCheck /mnt/disks/ssd0/agent/llvm-project/polly/test/Isl/CodeGen/multiple-codegens.ll

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.Sat, Apr 17, 9:06 AM
sdesmalen updated this revision to Diff 343185.Wed, May 5, 1:59 PM

Rebased patch.

This revision is now accepted and ready to land.Mon, May 10, 3:38 AM
fhahn added inline comments.Mon, May 10, 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

5733

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

5820

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.Mon, May 10, 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
5820

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.