[PATCH] D64916: [LV] isScalarEpilogueAllowed. NFC.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Generally, the patch looks fine. I'd would prefer if someone who has contributed to VPlan to accept the patch.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:848
 public:
-  LoopVectorizationCostModel(Loop *L, PredicatedScalarEvolution &PSE,
+  LoopVectorizationCostModel(bool isEpilogueAllowed, Loop *L, 
+                             PredicatedScalarEvolution &PSE,
----------------
[style] Use [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | PascalCase for variable/parameter ]] names.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4677
   if (Legal->getRuntimePointerChecking()->Need) {
-    ORE->emit(createMissedAnalysis("CantVersionLoopWithOptForSize")
+    ORE->emit(createMissedAnalysis("CantVersionLoopWithScalarEpilogueNotAllowed")
               << "runtime pointer checks needed. Enable vectorization of this "
----------------
Renaming `CantVersionLoopWithOptForSize` breaks API: Tools parsing the YAML output might match the string. I wouldn't rename these identifiers without a reason.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4795
   unsigned MaxVF = MaxVectorSize;
-  if (TTI.shouldMaximizeVectorBandwidth(OptForSize) ||
-      (MaximizeBandwidth && !OptForSize)) {
+  if (TTI.shouldMaximizeVectorBandwidth(!isScalarEpilogueAllowed()) ||
+      (MaximizeBandwidth && isScalarEpilogueAllowed())) {
----------------
Using the getter method for the object's own field looks strange. Did you consider directly using `IsScalarEpilogueAllowed`?


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

https://reviews.llvm.org/D64916