[PATCH] D96350: [SVE][LoopVectorize] Enable vectorization of fmin/fmax with nnan

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Thanks for working on this. I was headed this direction after D95690 <https://reviews.llvm.org/D95690>.
Using FMF instead of function attributes should be ok here, but we need to be careful about at least 2 things before we make this change:

1. The existing predicate is inadequate for fmin/fmax reductions. We should be requiring `nsz` too (or the function-level "no-signed-zeros-fp-math"="true"). The code as-is can miscompile because it is missing that check.
2. IR-level FMF are currently not fully propagated as we would like. They don't appear on load/store or function arguments. Because of that, we should do a union of flags between the fcmp and select (as shown in D95690 <https://reviews.llvm.org/D95690> too).



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:598
+    if (isFPMinMaxRecurrenceKind(Kind) &&
+        (HasFunNoNaNAttr || I->hasNoNaNs()))
+      return isMinMaxSelectCmpPattern(I, Prev);
----------------
david-arm wrote:
> Hi @kmclaughlin, I've not looked into this in the same detail as you have, but I wonder if the reason we only checked the function attribute previously is because the reduction is used outside the loop? Not saying that's a good reason for not vectorising though. :) Just that perhaps it was technically the easier solution since we might have to also look outside the loop to make sure users of the value also don't care about NaNs? Perhaps @dmgreen or @spatel have an idea?
I think the use of FP function attributes is historical. The instruction-level FMF were introduced later, and unfortunately they still are not adequate for all use cases:
https://llvm.org/PR38086
https://llvm.org/PR35607
https://llvm.org/PR35538



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96350