[PATCH] D96021: [LoopVectorize] NFC: Move UserVF feasibility checks to separate function.

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes accepted this revision.
c-rhodes added a comment.
This revision is now accepted and ready to land.

LGTM, might be worth leaving a little time before landing incase others have any comments, cheers



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5559
+
+  auto GetFeasibleMaxVF = [&]() -> ElementCount {
+    // First analyze the UserVF, fall back if the UserVF should be ignored.
----------------
sdesmalen wrote:
> c-rhodes wrote:
> > should functions start with lower case?
> GetFeasibleMaxVF is also a local variable (with a function as value) so I thought it should start with upper case. But it seems the code-base is a bit undecided; in a quick grep I counted slightly (15%) more cases starting with an upper-case, than starting with a lower-case.
> GetFeasibleMaxVF is also a local variable (with a function as value) so I thought it should start with upper case. But it seems the code-base is a bit undecided; in a quick grep I counted slightly (15%) more cases starting with an upper-case, than starting with a lower-case.

Ah fair enough, thanks for checking.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5749-5759
+  LLVM_DEBUG(dbgs() << "LV: User VF=" << UserVF
+                    << " is unsafe, clamping to max safe VF=" << MaxSafeVF
+                    << ".\n");
+  ORE->emit([&]() {
+    return OptimizationRemarkAnalysis(DEBUG_TYPE, "VectorizationFactor",
+                                      TheLoop->getStartLoc(),
+                                      TheLoop->getHeader())
----------------
sdesmalen wrote:
> c-rhodes wrote:
> > it's nice how you've reused the remark for the debug message above, not really important for this patch but it would be good to do the same thing here
> Thanks. I'm happy to see if I can do that in a separate patch (it requires some changes to the tests).
> Thanks. I'm happy to see if I can do that in a separate patch (it requires some changes to the tests).

I agree that would be better in a separate patch considering the changes to tests required.


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

https://reviews.llvm.org/D96021