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

Diego Caballero via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 9 21:47:31 PDT 2018


dcaballe marked 2 inline comments as done.
dcaballe added a comment.

Thanks, Florian! Some comments below.

> One thing worth discussing briefly before this goes in may be what the plan for dealing with debug info will be with VPlan. Adding @aprantl in case he has some thoughts.

I'm not aware of any particular proposal for debug info in VPlan at this point but I will check with my team. Currently, DbgInfoIntrinsic would be represented as a regular VPInstruction. We could think about if a specific representation for this is necessary in VPlan.



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:73
+  public:
+    /// \brief Creates a new insertion point which doesn't point to anything.
+    VPInsertPoint() = default;
----------------
fhahn wrote:
> There's been a few commits removing \brief from the codebase (e.g. D46290) recently, could you remove \brief from this patch, it should not be needed as we use AUTOBRIEF.
Thanks! I had no idea. I don't usually use \brief but most of this documentation is coming from IRBuilder.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:150
+    VPBuilder &Builder;
+    // TODO: AssertingVH<VPBasicBlock> Block;
+    VPBasicBlock* Block;
----------------
fhahn wrote:
> What does this comment mean?
`AssertingVH` is used in IRBuilder but we are not using it here. However, we may want to think about using something similar but I haven't look at it at all. We could do it as a separate patch if we are interested. I don't think we need this TODO. I'll remove it.


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:133
+bool PlainCFGBuilder::isExternalDef(Value *Val) {
+  // All the Values that are not Instructions are considered external
+  // definitions for now.
----------------
fhahn wrote:
> Could we use a similar (simpler) logic to what @hsaito  used in D46302 here? Like  Instr->getParent() strictly dominates the pre header?
Good point! I think the scenario is a bit different and there would be some corner cases that wouldn't work if we do `DT->properlyDominates(Instr->getParent(), PH)`. For example, any definition in the loop exit with a use within the HCFG wouldn't work. Imagine something like:

```
ph.inner:
   %0 = phi %1, %t

loop.body:
   ...

loop.exit:
   %t =
   %a = ...
         = %a ... 

```

If I'm not mistaken, uses of '%t' and %a would be classified as external definitions and they are not.

Make sense? Any other idea?



https://reviews.llvm.org/D44338





More information about the llvm-commits mailing list