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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


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


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


================
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) {
----------------
Ayal wrote:
> Is this change independent of this patch?
Split off, will submit separately, thanks!


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



================
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());
     }
----------------
Ayal wrote:
> ```
>       OpType = OperationType::FPMathOp;
>       FMFs = Op->getFastMathFlags();
> ```
> ?
Simplified, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:947
 
-#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   FastMathFlags getFastMathFlags() const {
     FastMathFlags Res;
----------------
Ayal wrote:
> 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
Done in 0b17e9d2859a with an assertion added.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157144