[PATCH] D38676: [LV] Model masking in VPlan, introducing VPInstructions

[Gil Rapaport via Phabricator via llvm-commits]

gilr added a comment.

In https://reviews.llvm.org/D38676#929411, @rengolin wrote:

> I finished my review, and apart from my two final comments, everything looks fine.
>
> Thanks for the hard work! :)


Excellent. Will upload a revised version shortly.
Thanks a lot for reviewing this!



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:8374
   VPBasicBlock *VPBB = new VPBasicBlock("Pre-Entry");
-  VPlan *Plan = new VPlan(VPBB);
+  Plan = new VPlan(VPBB);
+
----------------
rengolin wrote:
> So, `buildVPlans` will call `buildVPlan` for each VF, which means we'll call `new VPlan` multiple times, but now the `Plan` is a class global and will hold the last created plan's pointer. What is the purpose of this pointer?
The Plan class-global pointer is used for maintaining the current VPlan being constructed. But this indeed may be confusing - will instead propagate the Plan into the Planner's methods.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:241
+    // Bring the Values from ValueMap.
+    return Callback.getOrCreateVectorValues(VPValue2Value[Def], Part);
+  }
----------------
rengolin wrote:
> How is this saving the value in the `DenseMap PerPartOutput`?
This method is only a getter. Assigning Values to VPValues is done using set(). The old ILV ValueMap mechanism is delegated the call for Defs which are still not fully ported to the VPInstruction framework (and have therefore not been set()). I'll try to improve this method's comments to express this more clearly.


https://reviews.llvm.org/D38676