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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked an inline comment as done.
sepavloff added inline comments.


================
Comment at: llvm/docs/LangRef.rst:12931-12934
+'``llvm.roundeven.*``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Syntax:
----------------
andrew.w.kaylor wrote:
> 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.
> 
> 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.

Yes, unconstrained `llvm.rint` and `llvm.nearbyint` implement `roundToIntegralTiesToEven` in default mode, so they are not needed. I asked some time ago, if we need these intrinsics (http://lists.llvm.org/pipermail/cfe-dev/2020-March/064803.html). It looks like there is no reason to keep them, so they can be removed, only constrained versions should remain. Of unconstrained intrinsic only `llvm.roundeven` should survive as it is proposed exactly for this kind of rounding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75670