This is an archive of the discontinued LLVM Phabricator instance.

Don't raise inexact when lowering ceil, floor, round, trunc
ClosedPublic

Authored by scanon on Sep 18 2015, 9:39 AM.

Details

Summary

The C standard has historically not specified whether or not these functions should raise the inexact flag. Traditionally on Darwin, these functions *did* raise inexact, and the llvm lowerings followed that conventions. n1778 (C bindings for IEEE-754 (2008)) clarifies that these functions should not set inexact. This patch brings the lowerings for arm64 and x86 in line with the newly specified behavior.

Diff Detail

Repository
rL LLVM

Event Timeline

scanon updated this revision to Diff 35094.Sep 18 2015, 9:39 AM
scanon retitled this revision from to Don't raise inexact when lowering ceil, floor, round, trunc.
scanon updated this object.
scanon added a reviewer: t.p.northover.
scanon added a subscriber: llvm-commits.

Oh, nice! I wished we could do this when I became aware of the issue but thought it would be too late. The AArch64 changes look fine to me, and the x86 ones technically correct (still raise an exception for frint, for example).

It looks like this changes Linux behaviour on x86 though. I still think the change should be made (as does at least one GCC developer: https://gcc.gnu.org/ml/gcc/2015-01/msg00274.html), but perhaps we should give people a few days to pipe up if they object.

Tim.

gberry edited edge metadata.Sep 18 2015, 11:34 AM

I believe with this change we could remove both AArch64DAGToDAGISel::SelectFPConvertWithRound() and AArch64DAGToDAGISel::SelectLIBM() in favor of TD file patterns as well.

Quite right, well spotted!

Tim.

scanon updated this revision to Diff 35276.Sep 21 2015, 10:24 AM
scanon edited edge metadata.

Removed AArch64DAGToDAGISel::SelectFPConvertWithRound() and SelectLIBM() in favor of TD patterns.

Thanks for updating the patch Steve. One more thing I spotted:

lib/Target/AArch64/AArch64InstrInfo.td
2488–2492 ↗(On Diff #35276)

Sorry for only bringing this up now, but this doesn't look right. I think if an FRINTX does get created it probably should be treated specially. You'd mostly only do so (by calling "rint") if you actually cared about the otherwise unmodeled FPSR flags, wouldn't you?

Sorry for only bringing this up now, but this doesn't look right. I think if an FRINTX does get created it probably should be treated specially. You'd mostly only do so (by calling "rint") if you actually cared about the otherwise unmodeled FPSR flags, wouldn't you?

On x86 rint( ) is often just the fastest portable way to get round-to-nearest integer (assuming default rounding is set), so it's mostly used for that purpose, not because the caller is necessarily interested in the flags.

We don't handle FPSR like this anywhere else, so it doesn't seem like we should give it special attention here either. While I'd like to eventually model floating-point flags correctly, I don't think it's important that we go out of our way to add fake side-effects to one instruction in isolation.

t.p.northover accepted this revision.Sep 21 2015, 11:09 AM
t.p.northover edited edge metadata.

Fair point, and it does seem much less important now that we're not having to keep an otherwise dead FCVTX around for libm compatibility.

I think this looks good (again) now, then.

Anyone from the x86 Linux world object to following C1y's rules more widely? Speak up now if so.

Tim.

This revision is now accepted and ready to land.Sep 21 2015, 11:09 AM
This revision was automatically updated to reflect the committed changes.