[PATCH] D151482: [LV] Add support for minimum/maximum intrinsics

Tue Jun 13 11:57:37 PDT 2023

anna marked an inline comment as done.
anna added inline comments.

================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:814
+     // minimum and maximum intrinsics do not require nsz and nnan flags since
+     // signed zeroes and NaN are these are supported in the intrinsic
+     // implementation.
----------------
anna wrote:
> dmgreen wrote:
> > As far as I understand from the language ref, llvm.vector.reduce.fmin will be unspecified for 0.0/-0.0, and will return a value unless all the inputs are Nan. https://llvm.org/docs/LangRef.html#llvm-vector-reduce-fmin-intrinsic. This matches the behaviour of llvm.minnum. llvm.minimum has stronger semantics for signed zero and propagates nans, so would only be valid to convert with fast math flags.
> > 
> > I'm not 100% sure that llvm.minnum would need fast math flags, but llvm.minimum would seem to. Does it sound like I have that the right way around?
> > As far as I understand from the language ref, llvm.vector.reduce.fmin will be unspecified for 0.0/-0.0, and will return a value unless all the inputs are Nan.
> Ah, looks like vector.reduce.fmin was designed with only minnum in mind according to the langref: "This instruction has the same comparison semantics as the ‘llvm.minnum.*’ intrinsic. ".  Adding the flags `nnan` and `nsz` for llvm.minimum so that we can feed it into vector.reduce.fmin unfortunately defeats the purpose of these minimum intrinsic. 
> What we need is something like:
> ```
> %v = call <2 x float> @llvm.minimum.v2f32(...)
> ...
> %F = llvm.vector.reduce.fminimum(%v)
> ```
> where llvm.vector.reduce.fminimum follows the semantics of `llvm.minimum`. 
> I'm not sure if we can piggyback on the existing llvm.vector.reduce.fmin intrinsic or would need to create a new one. 
this is handled now - we have the new llvm.vector.reduce.fminimum/fmaximum intrinsics introduced. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151482