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

[fhahn]

Summary

Update VPInstruction to use VPRecipeWithIRFlags to manage FMFs for
VPInstruction.

A follow-up patch will also use VPRecipeWithIRFlags to remove the
CanonicalIVIncrementForPartNUW and CanonicalIVIncrementNUW opcodes.

[tschuett]

Thanks!. Have you ever considered to add something like getCalledFunction() when Opode == Instruction::Call to identify vscale?

[fhahn]

In D157144#4562654, @tschuett wrote:

Thanks!. Have you ever considered to add something like getCalledFunction() when Opode == Instruction::Call to identify vscale?

AFAIK vscale isn't currently modeled as VPInstruction yet, but once it does that certainly makes sense, thanks!

[tschuett]

In D157144#4563196, @fhahn wrote:

In D157144#4562654, @tschuett wrote:

Thanks!. Have you ever considered to add something like getCalledFunction() when Opode == Instruction::Call to identify vscale?

AFAIK vscale isn't currently modeled as VPInstruction yet, but once it does that certainly makes sense, thanks!

Given its importance maybe model it as a dedicated derived class of VPInstruction, e.g., VPVscaleInstruction.

[Ayal]

llvm/lib/Transforms/Vectorize/VPlan.h

839

Add a constructor and/or operator= from llvm::FastMathFlags?

863

Is this change independent of this patch?

863

Could FMFs be set at construction time, only, similar to other flags?

869–870

Can this also be simplified into IterT alone?

883

OpType = OperationType::FPMathOp;
FMFs = Op->getFastMathFlags();

?

962–966

Worth documenting. Can be called if OpType is other than FPMathOp, but then returns arbitrary results?

Possibly worth taking out of line to VPlan.cpp

1055

Can this be another constructor rather than static member, one that takes an FMF as another parameter?

1056

Should clone also copy the flags? (Independent of this patch)

[fhahn]

Address latest comments, thanks!

llvm/lib/Transforms/Vectorize/VPlan.h

839

Added, thanks!

863

Yes updated the patch! This requires adding new constructors for both VPRecipeWithIRFlags and VPInstruciton (and the same for each variant (i.e. wrap flags and others), but should be more in line with the existing mechanisms.

863

Split off, will submit separately, thanks!

869–870

Yes, Split off, will submit separately, thanks!

883

Simplified, thanks!

962–966

Done in 0b17e9d2859a with an assertion added.

1055

Updated to use separate constructor, thanks!

1056

Yes this needs updating, will do separately.

[Ayal]

llvm/lib/Transforms/Vectorize/VPlan.h

870

(Nice to see I is not set/needed as underlying?)

903

Add VPInstruction?

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

385

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()?

399–402

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()?

[fhahn]

llvm/lib/Transforms/Vectorize/VPlan.h

870

Yes, at the moment VPInstructions are completely independent of any underlying IR.

903

Added in af635a5547ec.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

385

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.

399–402

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?

[fhahn]

Address comments, thanks!

[Ayal]

llvm/lib/Transforms/Vectorize/VPlan.h

870

(Then perhaps it's setUnderlyingInstr() below can be dropped if unused.)

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

399–402

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?

[fhahn]

Add hasFastMathFlags helper

[fhahn]

llvm/lib/Transforms/Vectorize/VPlan.h

870

Ah right sorry it is needed for some recipes that use this constructor, just not for VPInstructions which use the different one.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

399–402

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).

[fhahn]

Fix build error, guard isFPMathOp with ifdef as it's only used in assertions.

[Ayal]

Looks good to me! Adding couple of final minor nits.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

235

nit: redundant empty line

398

nit: this is if and only if, can assert(hasFastMathFlags() == isFPMathOp() && "...").

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

for (VPRecipeBase *CurrentLink : Worklist.getArrayRef().drop_front()) {
                 isa<VPWidenCallRecipe>(CurrentLink)) &&
                CurrentLink->getOperand(2) == PreviousLinkV &&
                "expected a call where the previous link is the added operand");
 
         // If the instruction is a call to the llvm.fmuladd intrinsic then we
         // need to create an fmul recipe (multiplying the first two operands of
         // the fmuladd together) to use as the vector operand for the fadd
         // reduction.
-        VPInstruction *FMulRecipe =
-            new VPInstruction(Instruction::FMul, {CurrentLink->getOperand(0),
-                                                  CurrentLink->getOperand(1)});
-        FMulRecipe->setFastMathFlags(CurrentLinkI->getFastMathFlags());
+        VPInstruction *FMulRecipe = new VPInstruction(
+            Instruction::FMul,
+            {CurrentLink->getOperand(0), CurrentLink->getOperand(1)},
+            CurrentLinkI->getFastMathFlags());
         LinkVPBB->insert(FMulRecipe, CurrentLink->getIterator());
         VecOp = FMulRecipe;
       } else {
         if (RecurrenceDescriptor::isMinMaxRecurrenceKind(Kind)) {
           if (auto *Cmp = dyn_cast<VPWidenRecipe>(CurrentLink)) {
             assert(isa<CmpInst>(CurrentLinkI) &&
                    "need to have the compare of the select");
             continue;

llvm/lib/Transforms/Vectorize/VPlan.h

 
 private:
   struct ExactFlagsTy {
     char IsExact : 1;
   };
   struct GEPFlagsTy {
     char IsInBounds : 1;
   };
   struct FastMathFlagsTy {
     char AllowReassoc : 1;
     char NoNaNs : 1;
     char NoInfs : 1;
     char NoSignedZeros : 1;
     char AllowReciprocal : 1;
     char AllowContract : 1;
     char ApproxFunc : 1;
+
+    FastMathFlagsTy(const FastMathFlags &FMF);
   };
 
   OperationType OpType;
 
   union {
     WrapFlagsTy WrapFlags;
     ExactFlagsTy ExactFlags;
     GEPFlagsTy GEPFlags;
     FastMathFlagsTy FMFs;
     unsigned char AllFlags;
   };
 
 public:
   template <typename IterT>
   VPRecipeWithIRFlags(const unsigned char SC, IterT Operands)
       : VPRecipeBase(SC, Operands) {
     OpType = OperationType::Other;
     AllFlags = 0;
   }
 
   template <typename IterT>
   VPRecipeWithIRFlags(const unsigned char SC, IterT Operands, Instruction &I)
       : VPRecipeWithIRFlags(SC, Operands) {
     if (auto *Op = dyn_cast<OverflowingBinaryOperator>(&I)) {
       OpType = OperationType::OverflowingBinOp;
       WrapFlags = {Op->hasNoUnsignedWrap(), Op->hasNoSignedWrap()};
     } else if (auto *Op = dyn_cast<PossiblyExactOperator>(&I)) {
       OpType = OperationType::PossiblyExactOp;
       ExactFlags.IsExact = Op->isExact();
     } else if (auto *GEP = dyn_cast<GetElementPtrInst>(&I)) {
       OpType = OperationType::GEPOp;
       GEPFlags.IsInBounds = GEP->isInBounds();
     } else if (auto *Op = dyn_cast<FPMathOperator>(&I)) {
       OpType = OperationType::FPMathOp;
-      FastMathFlags FMF = Op->getFastMathFlags();
-      FMFs.AllowReassoc = FMF.allowReassoc();
-      FMFs.NoNaNs = FMF.noNaNs();
-      FMFs.NoInfs = FMF.noInfs();
-      FMFs.NoSignedZeros = FMF.noSignedZeros();
-      FMFs.AllowReciprocal = FMF.allowReciprocal();
-      FMFs.AllowContract = FMF.allowContract();
-      FMFs.ApproxFunc = FMF.approxFunc();
+      FMFs = Op->getFastMathFlags();
     }
   }
 
   template <typename IterT>
   VPRecipeWithIRFlags(const unsigned char SC, IterT Operands,
                       WrapFlagsTy WrapFlags)
       : VPRecipeBase(SC, Operands), OpType(OperationType::OverflowingBinOp),
         WrapFlags(WrapFlags) {}
 
+  template <typename IterT>
+  VPRecipeWithIRFlags(const unsigned char SC, IterT Operands,
+                      FastMathFlags FMFs)
+      : VPRecipeBase(SC, Operands), OpType(OperationType::FPMathOp),
+        FMFs(FMFs) {}
+
   static inline bool classof(const VPRecipeBase *R) {
     return R->getVPDefID() == VPRecipeBase::VPInstructionSC ||
            R->getVPDefID() == VPRecipeBase::VPWidenSC ||
            R->getVPDefID() == VPRecipeBase::VPWidenGEPSC ||
            R->getVPDefID() == VPRecipeBase::VPReplicateSC;
   }
 
   /// Drop all poison-generating flags.
   void dropPoisonGeneratingFlags() {
     // NOTE: This needs to be kept in-sync with
     // Instruction::dropPoisonGeneratingFlags.
     switch (OpType) {
     case OperationType::OverflowingBinOp:
void setFlags(Instruction *I) const {
     }
   }
 
   bool isInBounds() const {
     assert(OpType == OperationType::GEPOp &&
            "recipe doesn't have inbounds flag");
     return GEPFlags.IsInBounds;
   }
 
+  /// Returns true if the recipe has fast-math flags.
+  bool hasFastMathFlags() const { return OpType == OperationType::FPMathOp; }
+
   FastMathFlags getFastMathFlags() const;
 
   bool hasNoUnsignedWrap() const {
     assert(OpType == OperationType::OverflowingBinOp &&
            "recipe doesn't have a NUW flag");
     return WrapFlags.HasNUW;
   }
 
   bool hasNoSignedWrap() const {
enum {
     CanonicalIVIncrementForPart,
     BranchOnCount,
     BranchOnCond
   };
 
 private:
   typedef unsigned char OpcodeTy;
   OpcodeTy Opcode;
-  FastMathFlags FMF;
   DebugLoc DL;
 
   /// An optional name that can be used for the generated IR instruction.
   const std::string Name;
 
   /// Utility method serving execute(): generates a single instance of the
   /// modeled instruction. \returns the generated value for \p Part.
   /// In some cases an existing value is returned rather than a generated
   /// one.
   Value *generateInstruction(VPTransformState &State, unsigned Part);
 
+#if !defined(NDEBUG)
+  /// Return true if the VPInstruction is a floating point math operation, i.e.
+  /// has fast-math flags.
+  bool isFPMathOp() const;
+#endif
+
 protected:
   void setUnderlyingInstr(Instruction *I) { setUnderlyingValue(I); }
 
 public:
   VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands, DebugLoc DL,
                 const Twine &Name = "")
       : VPRecipeWithIRFlags(VPDef::VPInstructionSC, Operands), VPValue(this),
         Opcode(Opcode), DL(DL), Name(Name.str()) {}
 
   VPInstruction(unsigned Opcode, std::initializer_list<VPValue *> Operands,
                 DebugLoc DL = {}, const Twine &Name = "")
       : VPInstruction(Opcode, ArrayRef<VPValue *>(Operands), DL, Name) {}
 
   VPInstruction(unsigned Opcode, std::initializer_list<VPValue *> Operands,
                 WrapFlagsTy WrapFlags, DebugLoc DL = {}, const Twine &Name = "")
       : VPRecipeWithIRFlags(VPDef::VPInstructionSC, Operands, WrapFlags),
         VPValue(this), Opcode(Opcode), DL(DL), Name(Name.str()) {}
 
+  VPInstruction(unsigned Opcode, std::initializer_list<VPValue *> Operands,
+                FastMathFlags FMFs, DebugLoc DL = {}, const Twine &Name = "");
+
   VP_CLASSOF_IMPL(VPDef::VPInstructionSC)
 
   VPInstruction *clone() const {
     SmallVector<VPValue *, 2> Operands(operands());
     return new VPInstruction(Opcode, Operands, DL, Name);
   }
 
   unsigned getOpcode() const { return Opcode; }
 
   /// Generate the instruction.
   /// TODO: We currently execute only per-part unless a specific instance is
   /// provided.
bool hasResult() const {
     case VPInstruction::BranchOnCond:
     case VPInstruction::BranchOnCount:
       return false;
     default:
       return true;
     }
   }
 
-  /// Set the fast-math flags.
-  void setFastMathFlags(FastMathFlags FMFNew);
-
   /// Returns true if the recipe only uses the first lane of operand \p Op.
   bool onlyFirstLaneUsed(const VPValue *Op) const override {
     assert(is_contained(operands(), Op) &&
            "Op must be an operand of the recipe");
     if (getOperand(0) != Op)
       return false;
     switch (getOpcode()) {
     default:

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

FastMathFlags VPRecipeWithIRFlags::getFastMathFlags() const {
   Res.setNoInfs(FMFs.NoInfs);
   Res.setNoSignedZeros(FMFs.NoSignedZeros);
   Res.setAllowReciprocal(FMFs.AllowReciprocal);
   Res.setAllowContract(FMFs.AllowContract);
   Res.setApproxFunc(FMFs.ApproxFunc);
   return Res;
 }
 
+VPInstruction::VPInstruction(unsigned Opcode,
+                             std::initializer_list<VPValue *> Operands,
+                             FastMathFlags FMFs, DebugLoc DL, const Twine &Name)
+    : VPRecipeWithIRFlags(VPDef::VPInstructionSC, Operands, FMFs),
+      VPValue(this), Opcode(Opcode), DL(DL), Name(Name.str()) {
+  // Make sure the VPInstruction is a floating-point operation.
+  assert(isFPMathOp() && "this op can't take fast-math flags");
+}
+
 Value *VPInstruction::generateInstruction(VPTransformState &State,
                                           unsigned Part) {
   IRBuilderBase &Builder = State.Builder;
   Builder.SetCurrentDebugLocation(DL);
 
   if (Instruction::isBinaryOp(getOpcode())) {
     Value *A = State.get(getOperand(0), Part);
     Value *B = State.get(getOperand(1), Part);
case VPInstruction::BranchOnCount: {
     Builder.GetInsertBlock()->getTerminator()->eraseFromParent();
     return CondBr;
   }
   default:
     llvm_unreachable("Unsupported opcode for instruction");
   }
 }
 
+#if !defined(NDEBUG)
+bool VPInstruction::isFPMathOp() const {
+  // Inspired by FPMathOperator::classof. Notable differences are that we don't
+  // support Call, PHI and Select opcodes here yet.
+  return Opcode == Instruction::FAdd || Opcode == Instruction::FMul ||
+         Opcode == Instruction::FNeg || Opcode == Instruction::FSub ||
+         Opcode == Instruction::FDiv || Opcode == Instruction::FRem ||
+         Opcode == Instruction::FCmp;
+}
+#endif
+
 void VPInstruction::execute(VPTransformState &State) {
   assert(!State.Instance && "VPInstruction executing an Instance");
   IRBuilderBase::FastMathFlagGuard FMFGuard(State.Builder);
-  State.Builder.setFastMathFlags(FMF);
+  assert(hasFastMathFlags() == isFPMathOp() &&
+         "Recipe not a FPMathOp but has fast-math flags?");
+  if (hasFastMathFlags())
+    State.Builder.setFastMathFlags(getFastMathFlags());
   for (unsigned Part = 0; Part < State.UF; ++Part) {
     Value *GeneratedValue = generateInstruction(State, Part);
     if (!hasResult())
       continue;
     assert(GeneratedValue && "generateInstruction must produce a value");
     State.set(this, GeneratedValue, Part);
   }
 }
case VPInstruction::CanonicalIVIncrementForPart:
     break;
   case VPInstruction::BranchOnCount:
     O << "branch-on-count";
     break;
   default:
     O << Instruction::getOpcodeName(getOpcode());
   }
 
-  O << FMF;
   printFlags(O);
   printOperands(O, SlotTracker);
 
   if (DL) {
     O << ", !dbg ";
     DL.print(O);
   }
 }
 #endif
 
-void VPInstruction::setFastMathFlags(FastMathFlags FMFNew) {
-  // Make sure the VPInstruction is a floating-point operation.
-  assert((Opcode == Instruction::FAdd || Opcode == Instruction::FMul ||
-          Opcode == Instruction::FNeg || Opcode == Instruction::FSub ||
-          Opcode == Instruction::FDiv || Opcode == Instruction::FRem ||
-          Opcode == Instruction::FCmp) &&
-         "this op can't take fast-math flags");
-  FMF = FMFNew;
-}
-
 void VPWidenCallRecipe::execute(VPTransformState &State) {
   assert(State.VF.isVector() && "not widening");
   auto &CI = *cast<CallInst>(getUnderlyingInstr());
   assert(!isa<DbgInfoIntrinsic>(CI) &&
          "DbgInfoIntrinsic should have been dropped during VPlan construction");
   State.setDebugLocFromInst(&CI);
 
   for (unsigned Part = 0; Part < State.UF; ++Part) {
for (unsigned Part = 0; Part < State.UF; ++Part) {
     Value *Op0 = State.get(getOperand(1), Part);
     Value *Op1 = State.get(getOperand(2), Part);
     Value *Sel = State.Builder.CreateSelect(Cond, Op0, Op1);
     State.set(this, Sel, Part);
     State.addMetadata(Sel, &I);
   }
 }
 
+VPRecipeWithIRFlags::FastMathFlagsTy::FastMathFlagsTy(
+    const FastMathFlags &FMF) {
+  AllowReassoc = FMF.allowReassoc();
+  NoNaNs = FMF.noNaNs();
+  NoInfs = FMF.noInfs();
+  NoSignedZeros = FMF.noSignedZeros();
+  AllowReciprocal = FMF.allowReciprocal();
+  AllowContract = FMF.allowContract();
+  ApproxFunc = FMF.approxFunc();
+}
+
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 void VPRecipeWithIRFlags::printFlags(raw_ostream &O) const {
   switch (OpType) {
   case OperationType::PossiblyExactOp:
     if (ExactFlags.IsExact)
       O << " exact";
     break;
   case OperationType::OverflowingBinOp: