[PATCH] D137954: Enable roundeven.

Craig Topper via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 6 21:10:48 PST 2022


craig.topper 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);
----------------
arsenm wrote:
> 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
I didn't say we couldn't. Merely saying that someone will find that surprising and probably file a bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137954



More information about the llvm-commits mailing list