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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:769
          Def->getVPDefID() == VPRecipeBase::VPBranchOnMaskSC ||
          Def->getVPDefID() == VPRecipeBase::VPWidenMemoryInstructionSC;
 }
----------------
Ayal wrote:
> nit: unrelated to this patch, but wonder if the above can be improved.
I think it is not needed any longer now that `getDefinigRecipe` returns `VPRecipeBase` which already is a direct subclass of VPUser. I will remove it separately.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:931
 
-  VP_CLASSOF_IMPL(VPRecipeBase::VPWidenSC, VPValue::VPVWidenSC)
+  VP_CLASSOF_IMPL(VPRecipeBase::VPWidenSC)
 
----------------
Ayal wrote:
> nit: is `VPRecipeBase::` needed here but unneeded in the constructor above? Worth being consistent throughout, and hopefully cleanup more.
Consistently update all uses I could find to use `VPDef::` which is the place where the VPDef IDs are actually defined: 56ffd39c3da89dc94ed7d7122fe5765e09429289


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:92
+    VPVRecipeSC // A VPValue sub-class that is a VPRecipeBase and also inherits
+                // from VPValue
   };
----------------
Ayal wrote:
> 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?)
> Is this enumeration really needed? Is a VPValueSC always a live-in, i.e., a VPValue w/o a defining recipe?

`VPValueSC` is also used for VPValues defined by recipes that define multiple results. Those do not inherit from VPValue. I updated the comment to make it clearer.

> 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?)

Remove the `and also...` bit and added `///`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:96
   VPValue(Value *UV = nullptr, VPDef *Def = nullptr)
       : VPValue(VPValueSC, UV, Def) {}
   VPValue(const VPValue &) = delete;
----------------
Ayal wrote:
> 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.
I split up the constructors into separate ones for the 3 cases:

1) live in
2) def has multiple defined values
3) def is VPValue subclass

and added comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140848