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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:865
   VPRecipeWithIRFlags(const unsigned char SC, IterT Operands,
                       Instruction &I)
       : VPRecipeWithIRFlags(SC, Operands) {
----------------
Ayal wrote:
> 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.)
Ah right sorry it is needed for some recipes that use this constructor, just not for VPInstructions which use the different one.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:397
   IRBuilderBase::FastMathFlagGuard FMFGuard(State.Builder);
-  State.Builder.setFastMathFlags(FMF);
+  if (isFPMathOp())
+    State.Builder.setFastMathFlags(getFastMathFlags());
----------------
Ayal wrote:
> 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?
Added `VPRecipeWithIRFlags::hasFastMathFlags()` and use it during execute. Also added an assert using isFPMathOp to make sure they don't diverge (the original reason for using `isFPMathOp`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157144