[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

833

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

855

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

859

Is this change independent of this patch?

866

Can this also be simplified into IterT alone?

880

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

?

947

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

1022

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

1028

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

[fhahn]

Address latest comments, thanks!

llvm/lib/Transforms/Vectorize/VPlan.h

833

Added, thanks!

855

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.

859

Split off, will submit separately, thanks!

866

Yes, Split off, will submit separately, thanks!

880

Simplified, thanks!

947

Done in 0b17e9d2859a with an assertion added.

1022

Updated to use separate constructor, thanks!

1028

Yes this needs updating, will do separately.

[Ayal]

llvm/lib/Transforms/Vectorize/VPlan.h

867

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

887

Add VPInstruction?

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

379

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

382

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

867

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

887

Added in af635a5547ec.

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

379

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.

382

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

867

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

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

382

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

867

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

382

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

221

nit: redundant empty line

381

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 = VPInstruction::createFMFOperator(
+            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

struct WrapFlagsTy {
     char HasNSW : 1;
   };
   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;
   };
 
   OperationType OpType;
 
   union {
     WrapFlagsTy WrapFlags;
     ExactFlagsTy ExactFlags;
     GEPFlagsTy GEPFlags;
     FastMathFlagsTy FMFs;
     unsigned char AllFlags;
   };
 
+protected:
+  /// Set the fast-math flags.
+  void setFastMathFlags(FastMathFlags FMFNew);
+
 public:
   template <typename IterT>
-  VPRecipeWithIRFlags(const unsigned char SC, iterator_range<IterT> Operands)
+  VPRecipeWithIRFlags(const unsigned char SC, IterT Operands)
       : VPRecipeBase(SC, Operands) {
     OpType = OperationType::Other;
     AllFlags = 0;
   }
 
   template <typename IterT>
   VPRecipeWithIRFlags(const unsigned char SC, iterator_range<IterT> Operands,
                       Instruction &I)
       : VPRecipeWithIRFlags(SC, Operands) {
     if (auto *Op = dyn_cast<OverflowingBinaryOperator>(&I)) {
       OpType = OperationType::OverflowingBinOp;
       WrapFlags.HasNUW = Op->hasNoUnsignedWrap();
       WrapFlags.HasNSW = 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();
+      setFastMathFlags(Op->getFastMathFlags());
     }
   }
 
   static inline bool classof(const VPRecipeBase *R) {
     return 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:
public:
   }
 
   bool isInBounds() const {
     assert(OpType == OperationType::GEPOp &&
            "recipe doesn't have inbounds flag");
     return GEPFlags.IsInBounds;
   }
 
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   FastMathFlags getFastMathFlags() const {
     FastMathFlags Res;
     Res.setAllowReassoc(FMFs.AllowReassoc);
     Res.setNoNaNs(FMFs.NoNaNs);
     Res.setNoInfs(FMFs.NoInfs);
     Res.setNoSignedZeros(FMFs.NoSignedZeros);
     Res.setAllowReciprocal(FMFs.AllowReciprocal);
     Res.setAllowContract(FMFs.AllowContract);
     Res.setApproxFunc(FMFs.ApproxFunc);
     return Res;
   }
 
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   void printFlags(raw_ostream &O) const;
 #endif
 };
 
 /// This is a concrete Recipe that models a single VPlan-level instruction.
 /// While as any Recipe it may generate a sequence of IR instructions when
 /// executed, these instructions would always form a single-def expression as
 /// the VPInstruction is also a single def-use vertex.
-class VPInstruction : public VPRecipeBase, public VPValue {
+class VPInstruction : public VPRecipeWithIRFlags, public VPValue {
   friend class VPlanSlp;
 
 public:
   /// VPlan opcodes, extending LLVM IR with idiomatics instructions.
   enum {
     FirstOrderRecurrenceSplice =
         Instruction::OtherOpsEnd + 1, // Combines the incoming and previous
                                       // values of a first-order recurrence.
enum {
     CanonicalIVIncrementForPartNUW,
     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);
 
 protected:
   void setUnderlyingInstr(Instruction *I) { setUnderlyingValue(I); }
 
 public:
   VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands, DebugLoc DL,
                 const Twine &Name = "")
-      : VPRecipeBase(VPDef::VPInstructionSC, Operands), VPValue(this),
+      : 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) {}
 
   VP_CLASSOF_IMPL(VPDef::VPInstructionSC)
 
+  static VPInstruction *createFMFOperator(unsigned Opcode,
+                                          ArrayRef<VPValue *> Operands,
+                                          FastMathFlags FMF, DebugLoc DL = {},
+                                          const Twine &Name = "");
+
   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

 }
 
 void VPRecipeBase::moveBefore(VPBasicBlock &BB,
                               iplist<VPRecipeBase>::iterator I) {
   removeFromParent();
   insertBefore(BB, I);
 }
 
+VPInstruction *VPInstruction::createFMFOperator(unsigned Opcode,
+                                                ArrayRef<VPValue *> Operands,
+                                                FastMathFlags FMF, DebugLoc DL,
+                                                const Twine &Name) {
+  auto *I = new VPInstruction(Opcode, Operands, DL, Name);
+  // 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");
+  I->setFastMathFlags(FMF);
+  return I;
+}
+
 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");
   }
 }
 
 void VPInstruction::execute(VPTransformState &State) {
   assert(!State.Instance && "VPInstruction executing an Instance");
   IRBuilderBase::FastMathFlagGuard FMFGuard(State.Builder);
-  State.Builder.setFastMathFlags(FMF);
+  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::CanonicalIVIncrementForPartNUW:
     break;
   case VPInstruction::BranchOnCount:
     O << "branch-on-count ";
     break;
   default:
     O << Instruction::getOpcodeName(getOpcode());
   }
 
-  O << FMF;
+  printFlags(O);
 
   for (const VPValue *Operand : operands()) {
     O << " ";
     Operand->printAsOperand(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);
   }
 }
 
+void VPRecipeWithIRFlags::setFastMathFlags(FastMathFlags FMFNew) {
+  OpType = OperationType::FPMathOp;
+  FMFs.AllowReassoc = FMFNew.allowReassoc();
+  FMFs.NoNaNs = FMFNew.noNaNs();
+  FMFs.NoInfs = FMFNew.noInfs();
+  FMFs.NoSignedZeros = FMFNew.noSignedZeros();
+  FMFs.AllowReciprocal = FMFNew.allowReciprocal();
+  FMFs.AllowContract = FMFNew.allowContract();
+  FMFs.ApproxFunc = FMFNew.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: