[PATCH] D111555: [LoopVectorize] Add strict reduction support for fmuladd intrinsic

[Rosie Sumpter via Phabricator via llvm-commits]

RosieSumpter marked 4 inline comments as done.
RosieSumpter added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9622
+      }
+      VPReductionRecipe *RedRecipe =
+          new VPReductionRecipe(&RdxDesc, R, ChainOp, VecOps, CondOp, TTI);
----------------
fhahn wrote:
> dmgreen wrote:
> > Would it be possible to create a FMul VPInstruction and a VPReductionRecipe? That way the VPlan better represents the final instructions.
> +1, that should hopefully help to remove some of the special handling for the `FMulAdd` from codegen.
Hi @dmgreen, thanks for the suggestion. Would you (or @fhahn) mind elaborating a bit on what you would expect this to look like? I see the point about wanting the FMul instruction to be present in the VPlan, but having spoken to @david-arm about it it seems this might mean the VPReductionRecipe having two underlying instructions - is this what you would expect? Any pointers you have would be very useful!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9796
+        FMF = FPMO->getFastMathFlags();
+        State.Builder.setFastMathFlags(FMF);
+      }
----------------
paulwalker-arm wrote:
> I'm assuming this change will remain live after this point and thus affect nodes created after exiting this function? Which would be bad.  In any case I think that instead of calling `CreateBinOp` you can instead call `CreateFMulFMF` which will propagate the FMF flags for you.
Hi @paulwalker-arm thanks for the comments. It turns out this exposed a problem with fast-math flags not being propagated for ordered reductions, so I've added a patch for that here D112548


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9807
         NewRed = State.Builder.CreateBinOp(
-            (Instruction::BinaryOps)getUnderlyingInstr()->getOpcode(),
-            PrevInChain, NewVecOp);
+            (Instruction::BinaryOps)RdxDesc->getOpcode(Kind), PrevInChain,
+            NewVecOp);
----------------
fhahn wrote:
> This is a nice cleanup and could be split off as simple NFC.
I've added an NFC patch for this here D112547 


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

https://reviews.llvm.org/D111555