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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:833
   };
   struct FastMathFlagsTy {
     char AllowReassoc : 1;
----------------
Add a constructor and/or operator= from llvm::FastMathFlags?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:855
+  /// Set the fast-math flags.
+  void setFastMathFlags(FastMathFlags FMFNew);
+
----------------
Could FMFs be set at construction time, only, similar to other flags?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:859
   template <typename IterT>
-  VPRecipeWithIRFlags(const unsigned char SC, iterator_range<IterT> Operands)
+  VPRecipeWithIRFlags(const unsigned char SC, IterT Operands)
       : VPRecipeBase(SC, Operands) {
----------------
Is this change independent of this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:866
   template <typename IterT>
   VPRecipeWithIRFlags(const unsigned char SC, iterator_range<IterT> Operands,
                       Instruction &I)
----------------
Can this also be simplified into IterT alone?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:880
     } 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());
     }
----------------
```
      OpType = OperationType::FPMathOp;
      FMFs = Op->getFastMathFlags();
```
?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:947
 
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   FastMathFlags getFastMathFlags() const {
     FastMathFlags Res;
----------------
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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1022
 
+  static VPInstruction *createFMFOperator(unsigned Opcode,
+                                          ArrayRef<VPValue *> Operands,
----------------
Can this be another constructor rather than static member, one that takes an FMF as another parameter?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1028
   VPInstruction *clone() const {
     SmallVector<VPValue *, 2> Operands(operands());
     return new VPInstruction(Opcode, Operands, DL, Name);
----------------
Should clone also copy the flags? (Independent of this patch)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157144/new/

https://reviews.llvm.org/D157144