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

Sander de Smalen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 10 08:53:26 PDT 2021


sdesmalen added a comment.

Thanks for the review/comments!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:208
+/// vectorize with.
+struct MaxVFCandidates {
+  ElementCount FixedVF;
----------------
fhahn wrote:
> 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?
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!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5817
+  MaxVFCandidates MaxFactors = computeFeasibleMaxVF(TC, UserVF);
+  if (MaxFactors.FixedVF.isVector() && !MaxFactors.ScalableVF) {
+    ElementCount MaxFixedVF = MaxFactors.FixedVF;
----------------
fhahn wrote:
> 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
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98721/new/

https://reviews.llvm.org/D98721



More information about the llvm-commits mailing list