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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

I haven't been keeping up with this area as much as I'd have liked, but have some comments. Overall the approach seems ok to me.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7950
+  ~VPWidenMemoryInstructionRecipe() {
+    if (User)
+      delete User;
----------------
Can we use unique_ptr instead of a raw pointer to avoid repeating this?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:8020
     VFRange SubRange = {VF, MaxVF + 1};
-    VPlan *Plan = buildVPlan(SubRange);
+    VPlan *Plan = buildVPlan(SubRange, NeedDef);
     VPlans.push_back(Plan);
----------------
This can be combined into line below.


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:210
+  IRBuilder<> &Builder = State.Builder;
+  switch (getOpcode()) {
+  case Instruction::UDiv:
----------------
Can you instead check if it's a BinaryOperator instead of having to repeat these opcodes?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:736
+    for (auto &MapEntry : Value2VPValue)
+      delete MapEntry.second;
   }
----------------
Why delete the value from the map but leave the key?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:755
 
+  void addVPValue(Value &V) {
+    if (!Value2VPValue.count(&V))
----------------
Just make this take a pointer?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:756
+  void addVPValue(Value &V) {
+    if (!Value2VPValue.count(&V))
+      Value2VPValue[&V] = new VPValue();
----------------
This seems like it should be an assert instead? Why would a VPValue ever be created twice for a given Value and VPlan pair?


https://reviews.llvm.org/D38676