[PATCH] D57598: [VPLAN] Determine Vector Width programmatically.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6111
+    assert(isPowerOf2_32(VF) && "VF needs to be a power of two");
+    const auto Msg = [](const bool IsUserVF) -> const char * {
+      return IsUserVF ? "user VF " : "computed VF ";
----------------
Is the lambda necessary? You can just have `<< (UserVF ? "user VF" : "computed VF") <<` inline, I think


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6114
+    };
+    LLVM_DEBUG(dbgs() << "LV: Using " << Msg(UserVF) << VF << ".\n");
+    buildVPlans(VF, VF);
----------------
I think it would be clearer to say something along the lines of "LV: Using .. VF to build VPlans", to make it a bit clearer that VF is not necessarily what will be used for vectorization (e.g. VF == 1 means no vectorization).


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7157
+  // Also, do not attempt to vectorize if no vector code will be produced.
+  if (VPlanBuildStressTest || EnableVPlanPredication || VF.Width == 1)
     return false;
----------------
There is VectorizationFactor::Disabled(), which is used in other places. Could you use this here instead of VF.Widht == 1?


================
Comment at: test/Transforms/LoopVectorize/explicit_outer_detection.ll:71
 
 ; Case 2: Annotated outer loop WITHOUT vector width information doesn't have to
 ; be collected.
----------------
Comment needs updating, we now analyze loops without user VF too.


================
Comment at: test/Transforms/LoopVectorize/outer_loop_test1_no_explicit_vect_width.ll:1
+; RUN: opt -S -loop-vectorize -enable-vplan-native-path -mtriple aarch64-gnu-linux < %s | FileCheck %s
+; RUN: opt -S -loop-vectorize -enable-vplan-native-path -mtriple x86_64  < %s | FileCheck %s
----------------
This test will fail on targets not built with X86/AArch64 targets. The X86 version should go in test/Transforms/LoopVectorize/X86/ and the AArch64 one in test/Transforms/LoopVectorize/AArch64/


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

https://reviews.llvm.org/D57598