[PATCH] D104308: [VP] Add vector-predicated reduction intrinsics

[Simon Moll via Phabricator via llvm-commits]

simoll added a comment.

In D104308#2831225 <https://reviews.llvm.org/D104308#2831225>, @craig.topper wrote:

> In D104308#2830379 <https://reviews.llvm.org/D104308#2830379>, @frasercrmck wrote:
>
>> In D104308#2830045 <https://reviews.llvm.org/D104308#2830045>, @simoll wrote:
>>
>>> Disabling lanes is really what makes the difference between these and the regular reduction intrinsics.
>>> There is also the corner case that all lanes are disabled and i am unsure what the return value should be then. Any thoughts on that?
>>
>> Agreed. I'll update the docs accordingly.
>>
>> We have a couple of options for what happens when all lanes are disabled. For starters, it follows logically from the definition of the expansion into `reduce(select %mask, %v, %neutral)` that we just return the neutral element. So that's the "easiest" definition in that sense.
>>
>> We could return `undef` too which would be closer to how the other VP intrinsics work.
>>
>> As for other options, it wouldn't make sense to me to specify that we return any of the vector elements (e.g. `v[0]`) as they're not conceptually active. And I think `poison` wouldn't make any sense. Are those the only realistic options?
>>
>> In terms of hardware (which is orthogonal but may help guide us), RVV always takes a start value so we'd just return the neutral element; I admit I might be surreptitiously led by that. The lowering for returning the "neutral element" may be complicated on some targets, involving fetching the active length and perhaps doing some kind of scalar select. How would VE work?
>
> Could we add a start value operand to all of the intrinsics? fadd and fmul already have it. If the vectorizer doesn't have a value it can put the neutral value. On targets like RISCV we can use that neutral value directly since we need the scalar input anyway. For other targets they can detect that the argument is neutral and not use it if it doesn't mesh with their ISA.

Feeding back the discussions we had off Phabricator into the review thread:

1. We agreed to have scalar start value parameters in all vp reduction intrinsics (unlike `llvm.vector.reduce.*` which only have them where needed for non-reassociatable reductions). This makes them regular and there is no need to match the start value.
2. There appears to be an issue with the `%evl == 0` corner case in that the ISAs (RISC- V and VE) do not update the result register in that case. There would need to be a register constraint between the start value register and the result register to enforce this. This sure is an artifact of the intrinsic reaching into isel. However, I am not sure how big of a problem that really is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104308