[PATCH] D45336: Apply accumulator to fadd/fmul experimental vector reductions (PR36734)

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1577-1578
+  Value *Result = Builder.CreateExtractElement(TmpVec, Builder.getInt32(0));
+  if (Acc && !isa<UndefValue>(Acc))
+    Result = CreateReductionOp(Acc, Result);
+  return Result;
----------------
RKSimon wrote:
> ABataev wrote:
> > Why are you excluding `UndefValue` here? If `Acc` is `Undef`, the `Result` must be `Undef` too, no?
> Undef appears to have been used to ignore the accumulator.... @aemerson can you confirm please?
I think I missed out a detail when I wrote the langref, original motivation of the scalar accumulator argument was for the use in strictly ordered FP reductions only. I.e. when the intrinsic call has no FMF flags attached then the accumulator argument is used, otherwise if there are no FMF flags then the argument is meant to be ignored.

If we're talking about the semantics of the intrinsics: whether or not the accumulator is undef should have no effect on the codegen for fast reductions. If it did, as your patch implements, then @ABataev is right in that a value + undef = undef. We would then have to ensure that we generate identity values for the particular reduction kind in the cases where we don't have an accumulator.


Repository:
  rL LLVM

https://reviews.llvm.org/D45336