[PATCH] D157144: [VPlan] Replace FMF in VPInstruction with VPRecipeWithIRFlags (NFC).
Ayal Zaks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 8 08:30:50 PDT 2023
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
More information about the llvm-commits
mailing list