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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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)
 
----------------
tianshilei1992 wrote:
> 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?
Having backend interpretations of IR semantics is no good. We can have as many operations as needed and targets can request expansion as required.

The AMDGPU answer is complicated. For denormal handling it varies based on subtarget and address space. I’m not sure if they handle signaling nans correctly. For the ALU operations, it depends on the IEEE mode bit which I suspect is ignored by atomics


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127041