[PATCH] D157144: [VPlan] Replace FMF in VPInstruction with VPRecipeWithIRFlags (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:865
   VPRecipeWithIRFlags(const unsigned char SC, IterT Operands,
                       Instruction &I)
       : VPRecipeWithIRFlags(SC, Operands) {
----------------
fhahn wrote:
> Ayal wrote:
> > (Nice to see `I` is not set/needed as underlying?)
> Yes, at the moment VPInstructions are completely independent of any underlying IR.
(Then perhaps it's setUnderlyingInstr() below can be dropped if unused.)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:397
   IRBuilderBase::FastMathFlagGuard FMFGuard(State.Builder);
-  State.Builder.setFastMathFlags(FMF);
+  if (isFPMathOp())
+    State.Builder.setFastMathFlags(getFastMathFlags());
----------------
fhahn wrote:
> Ayal wrote:
> > Better have isFPMathOp() check if OpType == OperationType::FPMathOp, as done in getFastMathFlags()'s assert (which it tries to guard)?
> > 
> > (If so) Better have isFPMathOp() in VPRecipeWithIRFlags rather than VPInstruction, to assist potential future callers of VPRecipeWithIRFlags::getFastMathFlags()?
> At the moment, the subclasses of `VPRecipeWithIRFlags` don't have direct access to its internal fields, including `OpTypes` and it's the subclasses responsibility to make sure they don't access flags that are not available for the recipe. 
> 
> I think it would probably be worth keeping things locked down that way for now and check directly in VPInstruction if it is an op that has fast-math flags (we do something similar for the CanonicalIVIncrement. WDYT?
Keeping OpTypes and fields private and locked down inside VPRecipeWithIRFlags is fine, suggest to provide a public VPRecipeWithIRFlags::isFPMathOp() { return  OpType == OperationType::FPMathOp; } in order to support callers of public VPRecipeWithIRFlags::getFastMathFlags(), efficiently.

Can have a private VPInstruction::isFPMathOp() (or isFPMathOpcode()? Under #ifndef NDEBUG?) to support the assert upon construction only, less efficiently.

Sounds right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157144