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

[Renato Golin via Phabricator via llvm-commits]

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

Great, LGTM now, thanks!



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:8374
   VPBasicBlock *VPBB = new VPBasicBlock("Pre-Entry");
-  VPlan *Plan = new VPlan(VPBB);
+  Plan = new VPlan(VPBB);
+
----------------
gilr wrote:
> 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.
Right, I see, it's the *current* plan. Let's keep it as an argument for now and try to find a better pattern later. Thanks!


================
Comment at: lib/Transforms/Vectorize/VPlan.h:241
+    // Bring the Values from ValueMap.
+    return Callback.getOrCreateVectorValues(VPValue2Value[Def], Part);
+  }
----------------
gilr wrote:
> 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.
Ok, I thought this was a direct caching mechanism, now it makes sense.


https://reviews.llvm.org/D38676