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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added inline comments.


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


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.h:30
+#include "VPlanVerifier.h"
+#include "llvm/ADT/DenseMap.h"
+
----------------
fhahn wrote:
> Could be moved to .cpp?
I can remove this for now.


================
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()))) {
----------------
fhahn wrote:
> 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.
Good point. However, I don't think this operation is that generic. Sometimes we will need RPO, some others DFS, some others RPO or DFS but filtering some blocks... But I agree, at least we could start with a blocksDFS() range. Since this spans beyond this patch, could we address it in a separate patch?


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.cpp:91
+static void verifyRegion(const VPRegionBlock *Region) {
+  const VPBlockBase *Entry = Region->getEntry();
+  const VPBlockBase *Exit = Region->getExit();
----------------
fhahn wrote:
> I think some compilers will complain about Entry and Exit being unused when building without assertions
Good catch! I showed some warning with other similar cases but not here. Thanks!


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.h:36
+public:
+  // TODO: This class will have class member variables in the future.
+
----------------
fhahn wrote:
> I think this TODO does not really add much info.
OK. Let me remove it. I was just trying to justify why the 'verifyHierarchicalCFG' is not static. We will have class members with analysis information that will be used by this interface.


================
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
----------------
fhahn wrote:
> nit: we are not checking the generated code, so I think we could drop `-S` here and below.
Right, thanks!


https://reviews.llvm.org/D44338