Page MenuHomePhabricator

[FPEnv] Intrinsic llvm.roundeven
ClosedPublic

Authored by sepavloff on Mar 5 2020, 3:51 AM.

Details

Summary

This intrinsic implements IEEE-754 operation roundToIntegralTiesToEven,
and performs rounding to the nearest integer value, rounding halfway
cases to even. This intrinsic represents the missed case of IEEE-754
rounding operations and now llvm provides full support of the rounding
operations defined by the standard.

Diff Detail

Event Timeline

sepavloff created this revision.Mar 5 2020, 3:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2020, 3:51 AM
arsenm added inline comments.Mar 5 2020, 9:25 AM
llvm/docs/LangRef.rst
13163–13166

Isn't this the same as rint? Since the default rounding mode is assumed

13197

I've never heard of this C standard function?

asbirlea removed a subscriber: asbirlea.Mar 5 2020, 10:03 AM
efriedma added inline comments.Mar 5 2020, 10:25 AM
llvm/docs/LangRef.rst
13163–13166

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.

13197

It's part of TS 18661-1, which I think is proposed to be part of the next version of the C standard.

arsenm added inline comments.Mar 5 2020, 10:54 AM
llvm/docs/LangRef.rst
13163–13166

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

efriedma added inline comments.Mar 5 2020, 12:59 PM
llvm/docs/LangRef.rst
13163–13166

See https://reviews.llvm.org/rL299247 for recent history of this.

sepavloff marked an inline comment as done.Mar 5 2020, 8:11 PM
sepavloff added inline comments.
llvm/docs/LangRef.rst
13163–13166

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.

arsenm added inline comments.Mar 16 2020, 10:07 AM
llvm/docs/LangRef.rst
13163–13166

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

sepavloff marked an inline comment as done.Mar 17 2020, 7:32 AM
sepavloff added inline comments.
llvm/docs/LangRef.rst
13163–13166

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.

Although the patch appears large, most of it is made by copy-paste of llvm.round pieces.

Any feedback?

llvm/docs/LangRef.rst
13163–13166

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.

sepavloff marked an inline comment as done.Mar 30 2020, 8:45 PM
sepavloff added inline comments.
llvm/docs/LangRef.rst
13163–13166

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.

sepavloff updated this revision to Diff 258961.Apr 21 2020, 3:55 AM

Rebased patch

Replacement of llvm.nearbyint and llvm.rint with llvm.roundeven in the case of default FP environment is an optimization. If it is not done, these intrinsics will be converted into runtime library calls and the semantics expected by user will not be broken. While pragma STDC FENV_ACCESS is not fully supported, compiler should not do such optimization, because it would made functions nearbyint* and rint* unavailable.

This patch only introduces new intrinsic function that will be used to implement functions roundeven* defined in new C standard.

ychen added a subscriber: ychen.Apr 21 2020, 10:30 AM

I think the experimental_constrained_roundeven part is uncontroversial.

For the rest, there are two competing interests here:

  1. We don't want multiple intrinsics that represent the same thing.
  2. Transforming calls to roundeven/rint/nearbyint to one of the other functions is surprising, even if it's legal in some sense, and will likely break some practical use because we don't control the C library.

Violating (1) seems annoying, but not much of a practical problem; the few bits of code that actually care can easily check for three names instead of one. Violating (2) seems like fodder for weird bug reports I don't want to deal with. Given that, I think we should accept this patch as-is.

Any feedback?

sepavloff updated this revision to Diff 262347.Wed, May 6, 5:24 AM

Rebased patch

craig.topper added inline comments.Fri, May 8, 1:45 PM
llvm/lib/Analysis/ConstantFolding.cpp
1416

This should probably be with ceil/floor/trunc etc. further down?

1506

should constrained_roundeven be here?

llvm/test/Transforms/InstCombine/double-float-shrink-2.ll
84

This comment should be roundevenf

sepavloff updated this revision to Diff 263152.Mon, May 11, 5:47 AM

Updated patch

sepavloff marked 3 inline comments as done.Mon, May 11, 5:50 AM
sepavloff added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
1416

Sure.

1506

Indeed. Thank you!

sepavloff updated this revision to Diff 266040.Mon, May 25, 9:30 AM

Rebased patch

This revision is now accepted and ready to land.Mon, May 25, 11:50 AM
This revision was automatically updated to reflect the committed changes.