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

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

Thanks Diego, LGTM. I've left a few minor nits and I am not entirely sure about the DEBUG_TYPE. Please wait with committing a bit, in case anybody else has additional comments/thoughts.



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:81
+    /// Returns true if this insert point is set.
+    bool isSet() const { return (Block != nullptr); }
+
----------------
nit: no braces around returned expression.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:150
+    VPBuilder &Builder;
+    VPBasicBlock* Block;
+    VPBasicBlock::iterator Point;
----------------
nit: VPBasicBlock *Block?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7736
   assert(OrigLoop->empty() && "Inner loop expected.");
   // Width 1 means no vectorize, cost 0 means uncomputed cost.
   const VectorizationFactor NoVectorization = {1U, 0U};
----------------
"means no vectorization" ?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1042
+  // (operators '==' and '<').
+  SmallSet<VPValue *, 32> VPExternalDefs;
+
----------------
nit: 32 seems quite large


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:101
+    assert(isa<VPInstruction>(VPVal) && "Expected VPInstruction for phi node.");
+    auto * VPPhi = cast<VPInstruction>(VPVal);
+    assert(VPPhi->getNumOperands() == 0 &&
----------------
nit: auto *VPPhi


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:237
+  // 2. Scan the body of the loop in a topological order to visit each basic
+  // block after having visited its predecessor basic blocks.Create a VPBB for
+  // each BB and link it to its successor and predecessor VPBBs. Note that
----------------
nit: space after .


================
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.
----------------
dcaballe wrote:
> 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?
> 
Ah yes, we would have to account for instructions in the preheader and exit block separately. Then it would probably not simplify things much.


================
Comment at: lib/Transforms/Vectorize/VPlanValue.h:48
+  // Hold the underlying Value, if any, attached to this VPValue.
+  Value * UnderlyingVal;
+
----------------
Value *UnderlyingVal?


================
Comment at: lib/Transforms/Vectorize/VPlanVerifier.cpp:19
+
+#define DEBUG_TYPE "vplan-verifier"
+
----------------
I am not entirely sure how this will interact with `-debug-only`. IIUC if we do not use loop-vectorize here, those messages will be excluded from `-debug-only=loop-vectorize`. IMO it is convenient to get the complete picture with `-debug-only=loop-vectorize`.


================
Comment at: test/Transforms/LoopVectorize/vplan_hcfg_stress_test.ll:53
+
+attributes #0 = { norecurse nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
nit: those attributes are unnecessary I think


https://reviews.llvm.org/D44338