[PATCH] D127041: [LLVM] Add the support for fmax and fmin in atomicrmw instruction

[Shilei Tian via Phabricator via llvm-commits]

tianshilei1992 added inline comments.


================
Comment at: llvm/docs/LangRef.rst:10336-10337
 - fsub: ``*ptr = *ptr - val`` (using floating point arithmetic)
+-  fmax: ``*ptr = *ptr > val ? *ptr : val`` (using floating point comparison)
+-  fmin: ``*ptr = *ptr < val ? *ptr : val`` (using floating point comparison)
 
----------------
arsenm wrote:
> This is *not* what fmin/fmax do and ignores nan handling, and canonicalization
> 
> We plausibly need to have 3 different pairs:
>   - The libm behavior for fmin/fmax (which matches the misnamed llvm.minnum/maxnum intrinsics)
>   - The IEEE-754 2008 minnum/maxnum behavior which has different signaling nan behavior
>   - The IEEE-754 2019 minnum/maxnum behavior which inverts the nan behavior
> 
> 
> This absolutely needs to state which one it is if it's just one
> 
> 
Thanks for the information. My initial thought is we have a "unified" representation at the IR level, and leave the backend decide what should be the behavior. I'm not sure if one backend is gonna support more than one, but if that's the case, we would definitely need different representation. Based on https://en.cppreference.com/w/c/numeric/math/fmax, it looks like `libm`'s behavior is pretty similar to what we expect to see here. Speaking of that, AMDGPU already has some support for `atomic_fmax` and `atomic_fmin` via builtins. Which standard does it follow?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127041