[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#895867, @rengolin wrote:

> Would it be possible to split the patch, so that we can review them in a more concise way?


Hi Renato,

The patch is admittedly quite massive despite our deliberate minimization efforts.
To avoid introducing dead code into LV we're adding the new VPlan constructs along with a single, well-bounded use case.
Splitting the masking code in LoopVectorize.cpp is challenging: once VPlan takes care of the masks it must rewire them to the relevant Recipes (which are few).

Most of the changes/additions to the VPlan* files are rather simple and technical. The changes in LoopVectorize.cpp are the more complex part and deserve a more elaborate explanation, to be appended to the patch summary:

- createBlockInMask(), createEdgeMask(): these two methods moved from ILV to the Planner. Their code is essentially unchanged, except they now generate VPValues instead of Values. They are now called by Planner and passed down to mask-aware Recipes during VPlan construction.

- ILV::vectorizeMemoryInstruction() was so far called from VPWidenRecipe and generated the masks on-the-fly using createBlockInMask(). In this patch it is called by the new VPWidenMemoryInstructionRecipe, which provides the masks as an optional argument.

- VPBlendRecipe takes over non-loop phi's. It generates the same sequence of selects, only based on VPlan masks.

- VPBranchOnMaskRecipe now takes a VPValue mask.

Another aspect to notice is that until VPValues fully replace the existing scalar-IR-based ValueMap mechanism we effectively have two Def-Use graphs co-exist during vectorized code generation. Rewiring these two graphs together is done using
(a) Recipes taking VPValues and writing to ValueMap, and conversely
(b) VPValues that model Values still handled in ValueMap. The ILVCallback provides the bridge from VPlan’s DataState to ValueMap.



================
Comment at: lib/Transforms/Vectorize/VPlan.h:736
+    for (auto &MapEntry : Value2VPValue)
+      delete MapEntry.second;
   }
----------------
aemerson wrote:
> gilr wrote:
> > aemerson wrote:
> > > Why delete the value from the map but leave the key?
> > These are Values from the existing IR code - VPlan doesn't own them, just maintains a VPValue to represent them at the VPlan-level Def-Use graph.
> I meant why not erase() the entry from the map completely. Lleaving the key in the map but with a now invalid pointer doesn't seem right, unless I'm missing something.
The invalid entries will be destroyed immediately after on exiting the destructor, but it would indeed be good to add an explicit call to Value2VPValue.clear(), similar to LICM's runOnLoop(). Thanks!


https://reviews.llvm.org/D38676