[PATCH] D137954: Enable roundeven.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:1195
+  case ISD::FROUNDEVEN:
+    return LowerFROUND(Op, DAG);
   case ISD::FROUND: return LowerFROUND(Op, DAG);
----------------
craig.topper wrote:
> Leonc wrote:
> > arsenm wrote:
> > > Leonc wrote:
> > > > arsenm wrote:
> > > > > I'm perpetually confused by the variety of rounding functions.
> > > > > 
> > > > > llvm.rint => round to nearest integer, in current rounding mode (which is assumed to be round nearest even), which is implied by being non-constrained
> > > > > llvm.nearbyint => same as llvm.rint, except no FP exceptions. FP exceptions aren't supported with non-constrained intrinsics, so this distinction is pointless
> > > > > 
> > > > > llvm.round -> round, away from 0
> > > > > llvm.roundeven -> round halfway nearest 0
> > > > > 
> > > > > 
> > > > > so I think this isn't the same as round, but is supposed to be the same as llvm.rint (which we have 3 different names for)
> > > > I thought `llvm.roundeven` is supposed to always round to nearest even regardless of the rounding mode. Does our implementation of `llvm.rint` ignore the rounding mode?
> > > Yes. This is all legacy cruft from when people were imaging possible solutions to supporting strictfp in the distant future without an actual design in mind. The non-strict, regular floating point intrinsics all assume RTE rounding with no fp exceptions.
> > > 
> > > This cruft is bothering me; as a follow up, can you prepare a patch to deprecate the old intrinsics and auto-upgrade them so we're left with one?
> > Can do. I agree it's needlessly confusing.
> For targets that don't optimize them to inline sequences, wouldn't that cause calls library calls to mutate into a different library call? Functionally it would be correct in the default environment, but might be surprising.
> 
> Depending on which one you choose as canonical it could cause link errors. I don't think you could choose roundeven as canonical since its not in older libm.
Part of the problem is thinking the intrinsics have anything to do with libm. You can lower the intrinsic to a different name for the libcall


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137954