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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

A minor nit but LGTM. I don't have an aversion to "dead code" if it's going to be used in the near future, so perhaps to make the patch smaller split it into an initial patch to add VPlanValue.h and then the VPlanBuilder.h as well as the changes to the documentation. Those pieces seem uncontroversial to me, before moving onto the vectorizer. @mkuper does this make sense?



================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:33
+raw_ostream &llvm::operator<<(raw_ostream &OS, const VPValue &V) {
+  if (const VPInstruction *Instr = dyn_cast<VPInstruction>(&V))
+    Instr->print(OS);
----------------
shorter with `const  auto*` here


================
Comment at: lib/Transforms/Vectorize/VPlan.h:736
+    for (auto &MapEntry : Value2VPValue)
+      delete MapEntry.second;
   }
----------------
gilr wrote:
> 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!
Ah I'd gotten mixed up when I was looking at this originally. I'm ok with the original code, sorry for noise, although maybe it makes sense to store them as unique_ptrs?


https://reviews.llvm.org/D38676