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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9164
+  VPDT.recalculate(*Plan);
+  assert(VPlanVerifier::verifyPlanIsValid(*Plan, VPDT) && "VPlan is invalid");
+#endif
----------------
Ayal wrote:
> 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.
Done, thanks!


================
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;
----------------
Ayal wrote:
> Blend recipe should adhere to def-before-use?
> PredInstPHI should be considered a header phi?
> Worth having a test to check the verifier?
> Blend recipe should adhere to def-before-use?

Yep, updated the condition to not skip blend recipes

> PredInstPHI should be considered a header phi?

Not sure if it makes sense to classify PredInstPHI as header-phi, but the verifier needs extending so to check the incoming values dominate the end of the incoming block. Will put up a patch for that separately.


> Worth having a test to check the verifier?

Added initial tests in dc8e2ea9 and update the patch to show improvements.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:216
         if (UI->getParent() == VPBB) {
           if (RecipeNumbering[UI] < RecipeNumbering[&R]) {
             errs() << "Use before def!\n";
----------------
Ayal wrote:
> nit: can early-continue (independent of this patch).
I kept the order for now as is, so the check directly relates to the error message.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:226
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+          UI->dump();
+          errs() << "before\n";
----------------
I removed that unrelated change from the diff, will submit a separate patch for that.


================
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)
----------------
Ayal wrote:
> nit: can use utils to iterate over basic blocks only, now that regions needs not be assigned numbers.
Done, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140514