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

[Florian Hahn via Phabricator via llvm-commits]

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