[PATCH] D44338: [LV][VPlan] Build plain CFG with simple recipes for outer loops.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:275
+    cl::desc("Build VPlan for every supported loop nest in the function and "
+             "bail out (stress test the VPlan HCFG construction in the "
+             "VPlan-native vectorization path)."));
----------------
Please mention after what stage we bail out


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2378
   // Collect inner loops and outer loops without irreducible control flow. For
   // now, only collect outer loops that have explicit vectorization hints.
+  if (L.empty() || VPlanBuildStressTest ||
----------------
Please update the comment to mention VPlan stress testing.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:971
+      : VPBlockBase(VPRegionBlockSC, Name), Entry(nullptr), Exit(nullptr),
+      IsReplicator(IsReplicator) {}
 
----------------
nit: I think we should match the indent with VPBlockBase?


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:27
+#include "llvm/Analysis/LoopIterator.h"
+#include "llvm/Analysis/ScalarEvolution.h"
+
----------------
Do we need SE here? I could not find any uses.


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.h:30
+#include "VPlanVerifier.h"
+#include "llvm/ADT/DenseMap.h"
+
----------------
Could be moved to .cpp?


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.h:34
+
+class ScalarEvolution;
+class Loop;
----------------
Do we need SE here? I could not find any uses.


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.cpp:44
+  for (const VPBlockBase *VPB :
+       make_range(df_iterator<const VPBlockBase *>::begin(Region->getEntry()),
+                  df_iterator<const VPBlockBase *>::end(Region->getExit()))) {
----------------
Iterating over the blocks in a region seems a generic thing and it would probably be worth adding it to VPRegionBlock. At least VPRegionBlock::dumpRegion seems to be using a similar logic.


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.cpp:91
+static void verifyRegion(const VPRegionBlock *Region) {
+  const VPBlockBase *Entry = Region->getEntry();
+  const VPBlockBase *Exit = Region->getExit();
----------------
I think some compilers will complain about Entry and Exit being unused when building without assertions


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.cpp:117
+    const VPRegionBlock *TopRegion) const {
+  if (!EnableHCFGVerifier) {
+    return;
----------------
no brackets needed


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.h:36
+public:
+  // TODO: This class will have class member variables in the future.
+
----------------
I think this TODO does not really add much info.


================
Comment at: test/Transforms/LoopVectorize/vplan_hcfg_stress_test.ll:1
+; RUN: opt < %s -loop-vectorize -enable-vplan-native-path -vplan-build-stress-test -vplan-verify-hcfg -debug-only=vplan-verifier -disable-output -S 2>&1 | FileCheck %s -check-prefix=VERIFIER
+; RUN: opt < %s -loop-vectorize -enable-vplan-native-path -vplan-build-stress-test -debug-only=vplan-verifier -disable-output -S 2>&1 | FileCheck %s -check-prefix=NO-VERIFIER -allow-empty
----------------
nit: we are not checking the generated code, so I think we could drop `-S` here and below.


https://reviews.llvm.org/D44338