[PATCH] D140514: [VPlan] Use VPDominatorTree in VPlanVerifier .

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.
Herald added a subscriber: StephenFan.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9164
+  VPDT.recalculate(*Plan);
+  assert(VPlanVerifier::verifyPlanIsValid(*Plan, VPDT) && "VPlan is invalid");
+#endif
----------------
nit: can alternatively have the verifier construct a dominator tree? Once one is used for non-verification purposes, it could be passed in, to save a recalculation.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:210
         auto *UI = dyn_cast<VPRecipeBase>(U);
-        if (!UI || isa<VPHeaderPHIRecipe>(UI))
+        if (!UI || UI->isPhi())
           continue;
----------------
Blend recipe should adhere to def-before-use?
PredInstPHI should be considered a header phi?
Worth having a test to check the verifier?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:216
         if (UI->getParent() == VPBB) {
           if (RecipeNumbering[UI] < RecipeNumbering[&R]) {
             errs() << "Use before def!\n";
----------------
nit: can early-continue (independent of this patch).


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:243
   for (const VPBlockBase *VPB : Iter) {
-    BlockNumbering[VPB] = Cnt++;
     auto *VPBB = dyn_cast<VPBasicBlock>(VPB);
     if (!VPBB)
----------------
nit: can use utils to iterate over basic blocks only, now that regions needs not be assigned numbers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140514