Page MenuHomePhabricator

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

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



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

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

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?


Would be good to address?


Comment needs updating


Comment needs updating


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


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!


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!


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.