[PATCH] D75670: [FPEnv] Intrinsic llvm.roundeven

[Andy Kaylor via Phabricator via llvm-commits]

andrew.w.kaylor added inline comments.


================
Comment at: llvm/docs/LangRef.rst:12931-12934
+'``llvm.roundeven.*``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Syntax:
----------------
sepavloff wrote:
> arsenm wrote:
> > sepavloff wrote:
> > > efriedma wrote:
> > > > arsenm wrote:
> > > > > efriedma wrote:
> > > > > > arsenm wrote:
> > > > > > > Isn't this the same as rint? Since the default rounding mode is assumed 
> > > > > > The name is a hint that we should prefer to lower to a library function named "roundeven", if we can't generate an inline sequence.  Other than that, yes, it's identical.
> > > > > > 
> > > > > > I vaguely recall that I tried to merge llvm.rint and llvm.nearbyint at some point, but someone ran into issues with missing symbols.  I can try to dig it up if you think it's important.
> > > > > I don't think the intrinsic name should ever influence codegen that way. We should not have redundant intrinsics, especially for some weird lowering hint
> > > > See https://reviews.llvm.org/rL299247 for recent history of this.
> > > > Isn't this the same as rint? Since the default rounding mode is assumed
> > > 
> > > `roundeven` does not assume any mode. It always rounds to nearest, ties to even.
> > > If we eventually accept the viewpoint that default mode may be assumed always in the absence of `pragma FENV_ACCESS`, then it is more natural to replace `rint` with this intrinsic.
> > > 
> > > > The name is a hint that we should prefer to lower to a library function named "roundeven", if we can't generate an inline sequence. Other than that, yes, it's identical.
> > > 
> > > There is a library function `roundeven`, which of course differs from any other rounding function.
> > > 
> > > > I don't think the intrinsic name should ever influence codegen that way. We should not have redundant intrinsics, especially for some weird lowering hint
> > > 
> > > This is a function defined in the recent C standard draft http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2454.pdf, 7.12.9.8.
> > > 
> > > > See https://reviews.llvm.org/rL299247 for recent history of this.
> > > 
> > > It is semantically necessary change as `rint` and `nearbyint` are different functions if we are interested with FP exception status.
> > > If we eventually accept the viewpoint that default mode may be assumed always in the absence of pragma FENV_ACCESS, then it is more natural to replace rint with this intrinsic.
> > 
> > This is already very much the case. The regular FP operations and non constrained intrinsics assume the default FP environment
> > 
> >> If we eventually accept the viewpoint that default mode may be assumed always in the absence of pragma FENV_ACCESS, then it is more natural to replace rint with this intrinsic.
> > 
> > This is already very much the case. The regular FP operations and non constrained intrinsics assume the default FP environment
> 
> That's true. We need this intrinsic to implement `nearbyint` and `rint` in default mode.
> 
> 
> It is semantically necessary change as rint and nearbyint are different functions if we are interested with FP exception status.

In terms of LLVM IR, the unconstrained intrinsics llvm.rint, llvm.nearbyint, and llvm.roundeven are semantially identical. Backends should be able to lower them all the same way. If not, that's a bug. For this reason, I'm not sure we need all three.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75670