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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Just a few small initial comments. I'll have a closer look in the following days.



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:26
 
+extern cl::opt<bool> EnableVPlanPredication;
+
----------------
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.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1474
+    const VPLoop *ToLoop = VPLI->getLoopFor(ToBlock);
+    if (FromLoop == nullptr || ToLoop == nullptr || FromLoop != ToLoop) {
+      return false;
----------------
nit: I think LLVM style would be `if (!FromLoop || !ToLoop || FromLoop != ToLoop)`

Also, not braces for blocks with a single statement.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1477
+    }
+    // A back-edge is latch->header
+    return (ToBlock == ToLoop->getHeader() && ToLoop->isLoopLatch(FromBlock));
----------------
i think spelling it out would be clearer, e.g. a back-edge is a branch from the loop latch to its header.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1478
+    // A back-edge is latch->header
+    return (ToBlock == ToLoop->getHeader() && ToLoop->isLoopLatch(FromBlock));
+  }
----------------
outermost braces not needed?


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:29
+// Count PredBlock's successors, skipping back-edges
+int VPlanPredicator::countSuccessorsNoBE(VPBlockBase *PredBlock, bool &HasBE) {
+  HasBE = false;
----------------
personally I think it would be slightly clearer to have this function return a pair<int, bool>.

Also, the only state from VPlanPredicator used seems to be VPLI. Maybe this could just be a static non-member function?


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:31
+  HasBE = false;
+  int cnt = 0;
+  for (VPBlockBase *SuccBlock : PredBlock->getSuccessors()) {
----------------
would unsigned make more sense for cnt here? It should always be positive, right?


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:74
+  // Null BP means the predecesor is not predicated.
+  if (BP) {
+    VPValue *AndBP = Builder.createAnd(BP, IntermediateVal);
----------------
could be 

```
if (BP)
 return Builder.createAnd(BP, IntermediateVal);;

return IntermediateVal;
```
No need for a FinalVal variable.


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:147
+  llvm_unreachable("Broken getEdgeTypeBetween");
+  return EDGE_TYPE_UNINIT;
+}
----------------
no return after unreachable?


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:184
+    // edge predicates. We use the predecessor's block predicate instead.
+    if (NumPredSuccsNoBE == 1) {
+      IncomingPredicate = PredBlock->getPredicate();
----------------
nit: no braces for then block.


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:191
+          getOrCreateNotPredicate(cast<VPBasicBlock>(PredBlock), CurrBB);
+    } else {
+      llvm_unreachable("FIXME: switch statement ?");
----------------
nit: no braces for else block.


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:216
+  // before the current block's block predicate can be computed.
+  for (VPBlockBase *Block : make_range(RPOT.begin(), RPOT.end())) {
+    createOrPropagatePredicates(Block, Region);
----------------
nit: no braces .


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:266
+    : Plan(Plan), VPLI(&(Plan->getVPLoopInfo())) {
+  VPDomTree.recalculate(*(cast<VPRegionBlock>(Plan->getEntry())));
+}
----------------
sguggill wrote:
> Predicator is currently computing the dominator information for the top region. Once we start storing dominator information in a VPRegionBlock, we can avoid this recalculation.
I think it would be worth calling this out as a FIXME comment, as this is definitely something we should address sooner rather than later.


================
Comment at: lib/Transforms/Vectorize/VPlanValue.h:42
   friend class VPlanHCFGTransforms;
+  friend class VPlanPredicator;
   friend class VPBasicBlock;
----------------
I think I missed which private things are used in VPlanPredicator?


Repository:
  rL LLVM

https://reviews.llvm.org/D53349