[PATCH] D69563: [LV] Strip wrap flags from vectorized reductions

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D69563#1763331 <https://reviews.llvm.org/D69563#1763331>, @dantrushin wrote:

> In D69563#1763159 <https://reviews.llvm.org/D69563#1763159>, @Ayal wrote:
>
> > Good catch, binary operations that perform reduction must indeed be vectorized w/o wrap flags.
> >
> > But this should apply to all such operations that participate in the vectorized part of the loop. Note that 
> >  (1) there may be several such add/sub instructions, as in llvm/test/Transforms/LoopVectorize/reduction.ll tests, and
>
>
> Is there some existing API to find them all? Or I need to invite my own?


AFAIK such an API does not currently exist.

> Would not it be easier just to not copy wrap flags in widenInstruction() for all instructions [which I was shy to do initially :) ]  or it is too aggressive?

Loosing all wrap flags would be too aggressive.

> 
> 
>> (2) the last instruction in the loop along a reduction chain may not be one of these binary wrapping ops, but may instead be e.g. a select or phi as in llvm/test/Transforms/LoopVectorize/if-reduction.ll tests.
> 
> But as far as I understand, neither select nor phi carry no wrap flags? Or you're saying I need to look through them?

Right, need to look through select and phi nodes, and possibly other (trunc/sext) instructions.

> 
> 
>> All these instructions should be identified either late as done here in fixReduction(), or early e.g. when they receive a (new) VPWidenRecipe with a additional indicator that they must not wrap.
> 
> Doing this early would make me build reduction's instruction list in buildVPlanWithVPRecipies and for each instruction in list pass new flag to VPWidenRecipe and then (during execute()) to widenInstruction()
>  Again, adding this new flag to it.
>  Is it correct?

Yes, that may be an alternative implementation.

> 
> 
>> Patch should be accompanied by test(s), e.g., derived from pr43828, and the fixing of existing tests.
> 
> Oops, I had them initially, but lost during review update. I'll add them back.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69563