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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1542
+
+  auto CreateReductionOp = [&](Value *X, Value *Y) {
+    Value *Result;
----------------
1. `auto`->`auto &&`
2. It is a bad idea to capture everything by reference, most of the variables can be captured by value, like `Op` or  `RedOps`. It is better to use explicit capturing rather than implict.


================
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;
----------------
Why are you excluding `UndefValue` here? If `Acc` is `Undef`, the `Result` must be `Undef` too, no?


================
Comment at: test/CodeGen/Generic/expand-experimental-reductions.ll:102-117
+define float @fadd_f32_accum(float %accum, <4 x float> %vec) {
+; CHECK-LABEL: @fadd_f32_accum(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[RDX_SHUF:%.*]] = shufflevector <4 x float> [[VEC:%.*]], <4 x float> undef, <4 x i32> <i32 2, i32 3, i32 undef, i32 undef>
+; CHECK-NEXT:    [[BIN_RDX:%.*]] = fadd fast <4 x float> [[VEC]], [[RDX_SHUF]]
+; CHECK-NEXT:    [[RDX_SHUF1:%.*]] = shufflevector <4 x float> [[BIN_RDX]], <4 x float> undef, <4 x i32> <i32 1, i32 undef, i32 undef, i32 undef>
+; CHECK-NEXT:    [[BIN_RDX2:%.*]] = fadd fast <4 x float> [[BIN_RDX]], [[RDX_SHUF1]]
----------------
Could you commit these tests separately as NFC and then update them using your changes to see the real effect of your patch?


Repository:
  rL LLVM

https://reviews.llvm.org/D45336