[PATCH] D159200: [VPlan] Fold (MUL A, 1) -> A as VPlan2VPlan transform.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:803
+/// Returns true is \p V is constant one.
+static bool isConstantOne(VPValue *V) {
+  if (!V->isLiveIn())
----------------
Ayal wrote:
> nit: could become a method of VPValue?
I am planning to follow up by adding/extending PatternMatcher.h support for VPlan, which seems like an appropriate place for it to live


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:811
+/// Returns the llvm::Instruction opcode for \p R.
+static unsigned getOpcodeForRecipe(VPRecipeBase &R) {
+  if (auto *WidenR = dyn_cast<VPWidenRecipe>(&R))
----------------
Ayal wrote:
> Also handle VPInstruction via its getOpcode()?
> 
> nit: could become a method of VPRecipeBase?
Updated to also handle VPInstruction, thanks!

I am planning to follow up by adding/extending PatternMatcher.h support for VPlan, which seems like an appropriate place for it to live, in particular because there are various recipes that cannot be mapped to an  opcode at the moment or it is at least not entirely clear to which opcode they should be matched, e.g. interleave or canonical IV recipes.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:819
+
+/// Try to simplify recipe \p R.
+static void simplifyRecipe(VPRecipeBase &R) {
----------------
Ayal wrote:
> nit: would "constantFold" be more accurate than the general "simplify"?
> 
> Comment that collecting R's folded into their operands is left for subsequent DCE.
> 
> May be worth indicating if any change was made, at some point - relevant for VPlan2VPlan passes in general.
> 
> May be worth testing that Mul 1, 1 gets optimized into a live-in.
I think `constantFold` would be too narrow, as other 'instcombine' style simplifications would be a good fit here as needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159200