[PATCH] D44338: [LV][VPlan] Build plain CFG with simple recipes for outer loops.

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Mar 19 10:52:07 PDT 2018


fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7704
+    if (VPlanBuildStressTest)
+      return {UserVF, 0};
+
----------------
Could we here just return NoVectorization and get rid of the additional check in LoopVectorizePass::processLoop? Also, could we move the setting of UserVF in `plan` too? Otherwise it seems harder to keep track of what's going on and we set UserVF even for inner loops.

Also, I think ideally we would only bail out if the outer loop is not supported, but achieving that seems more trouble than it's worth.


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.h:12
+/// This file declare the class VPlanVerifier, which contains utility functions
+/// to check the consistency and invariants of a VPlan.
+///
----------------
dcaballe wrote:
> fhahn wrote:
> > Is there a place where those invariants are mentioned? It may be worth briefly stating here what checks are done by the verifier. ATM it looks like it checks the links between the blocks and regions of the VPlan.
> The invariants that we are currently checking are described in the documentation of 'verifyHierarchicalCFG' (just below). I think we could move them here so that we can reference them from different utility functions. For example:
> 
> ```
> /// This file declares the class VPlanVerifier, which contains utility functions
> /// to check the consistency of a VPlan. This includes the following kinds of
> /// invariants:
> ///
> /// 1. Region/Block invariants:
> ///   - Region's entry/exit block must have no predecessors/successors,
> ///     respectively.
> ///   - Block's parent must be the region immediately containing the block.
> ///   - Linked blocks must have a bi-directional link (successor/predecessor).
> ///   - All predecessors/successors of a block must belong to the same region.
> ///   - Blocks must have no duplicated successor/predecessor.
> ```
> 
> What do you think?
Thanks, sounds good to me.


Repository:
  rL LLVM

https://reviews.llvm.org/D44338





More information about the llvm-commits mailing list