[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/VPlanPredicator.cpp:244
+VPlanPredicator::VPlanPredicator(VPlan *Plan)
+    : Plan(Plan), VPLI(&(Plan->getVPLoopInfo())) {
+  // FIXME: Predicator is currently computing the dominator information for the
----------------
sguggill wrote:
> 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.
Ok, sounds good.


================
Comment at: lib/Transforms/Vectorize/VPlanPredicator.cpp:46
+  VPValue *IntermediateVal = nullptr;
+  if (ET == EdgeType::TRUE_EDGE)
+    // CurrBB is the true successor of PredBB - nothing to do here.
----------------
If we EdgeType only has TRUE_EDGE and FALSE_EDGE and we turn this into a switch, we will get a nice compiler warning about missing cases if we add more elements to EdgeType.


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

https://reviews.llvm.org/D53349