[PATCH] D98708: [LoopVectorize] relax FMF constraint for FP induction

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D98708#2631603 <https://reviews.llvm.org/D98708#2631603>, @david-arm wrote:

> Yeah I'm fine with that if @dmgreen is happy? It makes sense to be consistent with the RecurrenceDescriptor code. I think from what I understand clang will only generate IR that contains the reassoc flag if we've set all the appropriate frontend flags. Therefore, currently the only ambiguity at the moment is when hand-writing IR and using the `reassoc` flag without `nsz`, right?

That's correct. If we're looking at IR coming from clang, it will always have both flags at least. AFAICT, we don't apply any fast-math-flags in the front-end if someone uses "-fassociative-math" alone.

In D98708#2631662 <https://reviews.llvm.org/D98708#2631662>, @dmgreen wrote:

> I was thinking more in terms of Alive - how reassoc might imply nsz or not, mathematically. It seems it would not general, but may be mistaken.
>
> I'm not sure where nsz for a vanilla fadd reduction would be needed though. Reassoc might well be enough. For predication + inloop reductions (which no-one uses yet for floats) it would be important if the identity value was 0.0 and we generated:
>
>   %x = select %cond, %load, {0.0, 0.0,...}
>   %y = vecreduce.fadd(%x)
>   %z = fadd %x, %phi
>
> But if we change that to -0.0 instead, would there be another need of nsz?

I can't think of any other. If we go back to patches like D47335 <https://reviews.llvm.org/D47335>, we've been conservatively requiring nsz (because gcc did it + nobody has proven otherwise).

That patch included this test:

  ; ( A + C1 ) + ( B + -C1 )
  ; Verify this folds to 'A + B' with 'reassoc' and 'nsz' ('nsz' is required)

But I'm not seeing why nsz was needed there...

So I'm fine either way - we can leave this as-is, or follow-up by requiring nsz to be safer.


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

https://reviews.llvm.org/D98708