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

[Satish K Guggilla via Phabricator via llvm-commits]

sguggill added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:26
 
+extern cl::opt<bool> EnableVPlanPredication;
+
----------------
fhahn wrote:
> 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?
I have added issue 40034 and also added it to our meta ticket.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:534
+  /// Remove all the predecessor of this block.
+  void clearPredecessors() { Predecessors.clear(); }
+
----------------
fhahn wrote:
> 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? 
Yes, the verifier checks that successors/predecessors are in sync.


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:107
+  // The root is the last node in the worklist.
+  VPValue *Root = Worklist.front();
+
----------------
fhahn wrote:
> nit: just `return Worklist.front()`?
I typically write code this way so that it is easier to print the values during debugging. Unless you insist, I prefer the code this way.


================
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
----------------
fhahn wrote:
> 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?
As mentioned in the comments, the dominator information will be computed per region and stored in the region block so that clients that need the same can reuse it. This is not being done currently. This information in a region is expected to be kept up-to-date or computed on demand if it is no longer valid. 

VPLoopInfo is for the whole VPlan and we are currently computing it and storing it after building the HCFG. Once the dominator information becomes available in a region block, we can simply fetch it from a region and use the same. Until then we need to compute the dominator information.


================
Comment at: unittests/Transforms/Vectorize/VPlanPredicatorTest.cpp:14
+
+namespace llvm {
+namespace {
----------------
fhahn wrote:
> 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?
I will add a test that checks other cases.


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

https://reviews.llvm.org/D53349