[PATCH] D53349: [VPlan] Changes to implement VPlan based predication for VPlan-native path.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:489
 
-    // Check whether the BranchInst is a supported one. Only unconditional
-    // branches, conditional branches with an outer loop invariant condition or
-    // backedges are supported.
-    if (Br && Br->isConditional() &&
+    // Check whether the BranchInst is a supported one. If VPlan-Predication is
+    // disabled only unconditional branches, conditional branches with an outer
----------------
IMO the comment would be easier to parse if we keep the original comment and add an additional sentence like: FIXME: Currently those checks are disabled when VPlan based predication is disabled.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:26
 
+extern cl::opt<bool> EnableVPlanPredication;
+
----------------
sguggill wrote:
> fhahn wrote:
> > Would it be possible to avoid adding a EnableVPlanPredication option? IIUC, we would always like to run predication, if it is required?
> > 
> > I think it would be good to avoid adding too many options, to ensure we get good test coverage for all parts. Or make the default true.
> Yes, I will remove this switch once we have divergence analysis. Right now when predication is enabled, we suppress the check for trivially uniform non-backedge branches and assume they are divergent. Without this check, the CG tests added testing uniform control flow will fail. I understand the concern about adding too many options and will add a FIXME comment about the same.
Right, that makes sense. Maybe we could use bugzilla to keep track of those things? Otherwise we might forget. Could you file an issue?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:534
+  /// Remove all the predecessor of this block.
+  void clearPredecessors() { Predecessors.clear(); }
+
----------------
IIUC it is the responsibility of the caller to remove the block from a predecessor's successor list? Does the verifier check that successors/predecessors are in sync? 


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1517
+    assert(FromBlock->getParent() == ToBlock->getParent() &&
+           FromBlock->getParent() != nullptr && "Must be in same region");
+    const VPLoop *FromLoop = VPLI->getLoopFor(FromBlock);
----------------
I think LLVM's convention is to not use `Foo != nullptr`, but just `Foo`. 


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:107
+  // The root is the last node in the worklist.
+  VPValue *Root = Worklist.front();
+
----------------
nit: just `return Worklist.front()`?


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:180
+  VPValue *Predicate = nullptr;
+  if (IncomingPredicates.size())
+    Predicate = genPredicateTree(IncomingPredicates);
----------------
not needed I think, as this is the first check in genPredicateTree too


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:244
+VPlanPredicator::VPlanPredicator(VPlan *Plan)
+    : Plan(Plan), VPLI(&(Plan->getVPLoopInfo())) {
+  // FIXME: Predicator is currently computing the dominator information for the
----------------
Is there a reason why we treat VPLoopInfo and VPDomTree differently here; i.e. fetch one from Plan and recompute the other? In a way both are tied together (if the DT changes, LoopInfo potentially also changes), so could we treat them in the same way, either fetch them both from Plan or recompute them here?


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.h:22
+#include "VPlanDominatorTree.h"
+#include "llvm/IR/Dominators.h"
+
----------------
Is this needed here? I cannot see any references to stuff defined in the generic dominators.h 


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.h:28
+private:
+  enum EdgeType {
+    EDGE_TYPE_UNINIT = 0,
----------------
could this be an enum class? That has the advantage that the type system guarantees that its value can only have predefined values.


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.h:36
+  // VPlan being predicated.
+  VPlan *Plan;
+
----------------
We always have a Plan here, could it be a reference?


================
Comment at: unittests/Transforms/Vectorize/VPlanPredicatorTest.cpp:14
+
+namespace llvm {
+namespace {
----------------
It would be good to have tests to cover the important cases. It looks like this test only checks the AND predicate generation. Could you also test: generation of negative predicates, generation of ORs?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D53349