[PATCH] D157144: [VPlan] Replace FMF in VPInstruction with VPRecipeWithIRFlags (NFC).
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 8 05:23:26 PDT 2023
fhahn marked 4 inline comments 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:
> (Nice to see `I` is not set/needed as underlying?)
Yes, at the moment VPInstructions are completely independent of any underlying IR.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:892
R->getVPDefID() == VPRecipeBase::VPWidenGEPSC ||
R->getVPDefID() == VPRecipeBase::VPReplicateSC;
}
----------------
Ayal wrote:
> Add VPInstruction?
Added in af635a5547ec.
================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:387
+bool VPInstruction::isFPMathOp() const {
+ return Opcode == Instruction::FAdd || Opcode == Instruction::FMul ||
----------------
Ayal wrote:
> Suffice to assert that Opcode is one having FMF, only when the Opcode and FMF are provided to VPInstruction's constructor, and its OpType is set?
>
> Check inspired by / should match / worth comment FPMathOperator:: classof()?
Added a note about the relation to `FPMathOperator::classof()` and the current differences. I've kept the function for now, as per my response to the comment below.
================
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:
> 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?
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