[PATCH] D113125: [LoopVectorize] Propagate fast-math flags for VPInstruction

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1214
+  O << " +";
+  if (isa<FPMathOperator>(getUnderlyingInstr()))
+    O << getUnderlyingInstr()->getFastMathFlags();
----------------
That change is not really related to the patch title/description. I think it would be good to either adjust the title or commit it separately.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:876
+  // Set the fast-math flags.
+  void setFastMathFlags(FastMathFlags FMFNew) { FMF = FMFNew; }
 };
----------------
RosieSumpter wrote:
> fhahn wrote:
> > can we assert that this is only used for opcodes that can take fast-math flags? On the IR side this is done by checking for `FPMathOperator`, but there's no helper to check if an opcode is a FP operation. Perhaps it would be sufficient to check for the relevant opcodes?
> > 
> > 
> > Also, for now we only need to set fast-math flags at construction time I think, so could this be set directly in the constructor?
> Hey @fhahn, thanks for this, I've added an assert to check for all the fp opcodes in ##setFastMathFlags##. I haven't set the fast-math flags directly in the constructor for now since the ##setFastMathFlags## approach makes the flag setting explicit, and mirrors the Instruction class. As always though, I'm happy to change it if you still think setting the flags in the constructor is the best way.
Thanks for adding the assert! For VPInstructions we do not need the full flexibility provided by `setFastMathFlags`, as for now we only setting them at construction time. This was the main reason to suggest doing it directly in the constructor. But I guess in the future this may be helpful in case we need to drop fast-math flags.


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

https://reviews.llvm.org/D113125