[PATCH] D112548: [LoopVectorize] Propagate fast-math flags for ordered reductions

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Thanks for this patch @RosieSumpter - seems like a nice fix! I wonder if it's also worth changing createOrderedReduction to be similar to createTargetReduction and set the flags in there too? Also, you've fixed up the VF=1 cases too.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9752
   bool IsOrdered = State.ILV->useOrderedReductions(*RdxDesc);
+  FastMathFlags Current_FMF = State.Builder.getFastMathFlags();
+  if (IsOrdered) {
----------------
I completely forgot we can actually just use a guard here, i.e.

  IRBuilderBase::FastMathFlagGuard FMFGuard(State.Builder);
  State.Builder.setFastMathFlags(RdxDesc->getFastMathFlags());

then you don't need the extra line at the bottom anymore to reset the flags.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9753
+  FastMathFlags Current_FMF = State.Builder.getFastMathFlags();
+  if (IsOrdered) {
+    // Propagate the fast-math flags carried by the underlying instruction.
----------------
Maybe we should do this for all cases here, i.e. ordered and non-ordered?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9755
+    // Propagate the fast-math flags carried by the underlying instruction.
+    if (auto *FPMO = dyn_cast<FPMathOperator>(getUnderlyingInstr())) {
+      FastMathFlags FMF = FPMO->getFastMathFlags();
----------------
I just realised that you might be able to replace the whole if block with:

  State.Builder.setFastMathFlags(RdxDesc->getFastMathFlags());

here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9779
       else
         NewRed = State.Builder.CreateBinOp(
             (Instruction::BinaryOps)RdxDesc->getOpcode(Kind), PrevInChain,
----------------
I just realised you're actually also fixing the case where VF=1. Is it worth having a test to ensure flags are propagated for VF=1 as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112548