[PATCH] D117580: [LoopVectorize] Support conditional in-loop vector reductions

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalable-reduction-inloop-cond.ll:26
+; CHECK-NEXT:    [[TMP8:%.*]] = bitcast float* [[TMP7]] to <vscale x 4 x float>*
+; CHECK-NEXT:    [[WIDE_MASKED_LOAD:%.*]] = call <vscale x 4 x float> @llvm.masked.load.nxv4f32.p0nxv4f32(<vscale x 4 x float>* [[TMP8]], i32 4, <vscale x 4 x i1> [[TMP6]], <vscale x 4 x float> zeroinitializer)
+; CHECK-NEXT:    [[TMP9]] = call fast float @llvm.vector.reduce.fadd.nxv4f32(float [[VEC_PHI]], <vscale x 4 x float> [[WIDE_MASKED_LOAD]])
----------------
david-arm wrote:
> I think this code looks right because the inactive lanes will be zero, which matches the identity value for an fadd. However, it might be clearer if you remove the `-dce -instcombine` flags so we can see the select? I assume the select has been folded away.
The select has been folded away, I've removed the flags from this test so that it's hopefully a bit clearer.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-inloop-cond.ll:234
+
+define i32 @multiple_cond_ands(i32* noalias %A, i32* noalias %B, i32 %cond, i64 noundef %N) #0 {
+; CHECK-LABEL: @multiple_cond_ands(
----------------
david-arm wrote:
> Hi @kmclaughlin, it doesn't look there are multiple conditional and instructions in the loop?
Hi @david-arm, I've renamed this to `@unconditional_and` since there's only one and in the loop.


================
Comment at: llvm/test/Transforms/LoopVectorize/reduction-inloop-cond.ll:689
+; Chain of conditional & unconditional reductions. We do not vectorise as the Phi (%rdx1) has more than
+; the expected number of uses.
+;
----------------
david-arm wrote:
> Perhaps worth adding a bit more here, i.e.
> 
>   ; Chain of conditional & unconditional reductions. We currently only support conditional reductions
>   ; if they are the last in the chain, i.e. the loop exit instruction is a PHI node. Therefore, we reject the
>   ; PHI (%rdx1) as it has more than one use.
> 
> Do you think that makes it a bit clearer?
I think that's clearer, thanks!


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

https://reviews.llvm.org/D117580