[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