[PATCH] D76992: [VPlan] Add & use VPValue operands for VPWidenRecipe (NFC).

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4285
     bool InvariantCond =
-        SE->isLoopInvariant(PSE.getSCEV(I.getOperand(0)), OrigLoop);
+        SE->isLoopInvariant(PSE.getSCEV(GetUnderlyingValue(0)), OrigLoop);
     setDebugLocFromInst(Builder, &I);
----------------
Ayal wrote:
> Better for a VPlan recipe to record such information/decision what Select should eventually be generated, rather than for code-gen to check the SCEV of an underlying value of an operand.
> 
> Perhaps worth taking the Select case out of ILV::widenInstruction() and into a dedicated ILV::widenSelectInstruction() which will be given InvariantCond as a parameter, and have VPWidenRecipe (or a dedicated VPWidenSelectRecipe) record InvariantCond and feed it?
> Better for a VPlan recipe to record such information/decision what Select should eventually be generated, rather than for code-gen to check the SCEV of an underlying value of an operand.
> 
> Perhaps worth taking the Select case out of ILV::widenInstruction() and into a dedicated ILV::widenSelectInstruction() which will be given InvariantCond as a parameter, and have VPWidenRecipe (or a dedicated VPWidenSelectRecipe) record InvariantCond and feed it?

Agreed. I originally planned on doing that as a follow-up, but it's probably easier to do up-front: D77869


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanValue.h:47
   friend class VPSlotTracker;
+  friend class VPWidenRecipe;
 
----------------
gilr wrote:
> fhahn wrote:
> > gilr wrote:
> > > Make ILV friends with VPValue instead? Would facilitate removing that last lambda.
> > I think granting access to ILV would mean that the scope where the underlying value can be accessed would be extended a lot and might encourage more uses (which I think is undesired). I think the lambda allows us to grant more targeted access and there is only a single use in widenInstruction, which will hopefully removed in the future. Does that make sense? I would be also happy to make ILV a friend, if you think that's preferable over the lambda in the long run.
> Letting ILV access the underlying value seems to follow the intent of the design principle preceding getUnderlyingValue()'s declaration of limiting its use to the code-gen phase.
> Anyway, I agree with Ayal that a better solution would be to take that decision during planning rather than during code-gen.
> Anyway, I agree with Ayal that a better solution would be to take that decision during planning rather than during code-gen.

+1
 I originally planned on doing that as a follow-up, but it's probably easier to do up-front: D77869



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76992