[PATCH] D140848: [VPlan] Remove duplicated VPValue IDs (NFCI).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Nice cleanup!



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:769
          Def->getVPDefID() == VPRecipeBase::VPBranchOnMaskSC ||
          Def->getVPDefID() == VPRecipeBase::VPWidenMemoryInstructionSC;
 }
----------------
nit: unrelated to this patch, but wonder if the above can be improved.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:931
 
-  VP_CLASSOF_IMPL(VPRecipeBase::VPWidenSC, VPValue::VPVWidenSC)
+  VP_CLASSOF_IMPL(VPRecipeBase::VPWidenSC)
 
----------------
nit: is `VPRecipeBase::` needed here but unneeded in the constructor above? Worth being consistent throughout, and hopefully cleanup more.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:92
+    VPVRecipeSC // A VPValue sub-class that is a VPRecipeBase and also inherits
+                // from VPValue
   };
----------------
Is this enumeration really needed? Is a VPValueSC always a live-in, i.e., a VPValue w/o a defining recipe?
>From the summary: "... only retain the generic VPValue ID (= used VPValues without a
corresponding defining recipe) ..."

nit: "A VPValue sub-class that is a VPRecipeBase and also inherits from VPValue" - Any VPValue sub-class surely also inherits from VPValue... (Worth /// if needed?)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:96
   VPValue(Value *UV = nullptr, VPDef *Def = nullptr)
       : VPValue(VPValueSC, UV, Def) {}
   VPValue(const VPValue &) = delete;
----------------
Should the "VPValueSC" constructor of VPValue above accept only a Value* as a single optional operand,
and the "VPVRecipeSC" constructor below be called whenever a (non-null) VPDef* is provided?
Deserves a comment.
Also worth indicating in the commit message that the operands are swapped to facilitate an optional underlying value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140848/new/

https://reviews.llvm.org/D140848