Page MenuHomePhabricator

[builtins] Rounding mode support for addxf3/subxf3
ClosedPublic

Authored by kongyi on Jan 24 2019, 12:39 AM.

Details

Summary

Implement rounding mode support for addxf3/subxf3.

On architectures that implemented the support, this will access the corresponding floating point environment register to apply the correct rounding. For other architectures, it will keep the current behaviour and use IEEE-754 default rounding mode (to nearest, ties to even).

ARM32/AArch64 support implemented in this change. i386 and AMD64 will be added in a follow up change.

Diff Detail

Event Timeline

kongyi created this revision.Jan 24 2019, 12:39 AM
kongyi updated this revision to Diff 183262.Jan 24 2019, 12:43 AM
efriedma added inline comments.
compiler-rt/lib/builtins/arm/fp_mode.c
25

This needs to be guarded; vmrs is only valid on targets which support VFP. Not sure if we need to try to check that at runtime...

MaskRay added inline comments.
compiler-rt/lib/builtins/arm/fp_mode.c
25

On ARM, perhaps we can assume only FE_TONEAREST is available.

https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/arm/ieee754-sf.S

Only the default rounding mode is intended for best performances.

MaskRay added inline comments.Jan 25 2019, 2:15 AM
compiler-rt/lib/builtins/arm/fp_mode.c
25

For aarch64, implementing __fegetround in assembly also looks good to me. In musl http://git.musl-libc.org/cgit/musl/tree/src/fenv/aarch64/fenv.s

.global fegetround
.type fegetround,%function
fegetround:
	mrs x0, fpcr
	and w0, w0, #0xc00000
	ret
joerg added a subscriber: joerg.Jan 25 2019, 4:27 PM

This seems to imply hard-float support though? Accessing floating point registers when using soft-float is not going to get very far.

kongyi updated this revision to Diff 183858.Jan 28 2019, 7:35 AM
kongyi marked 2 inline comments as done.Jan 28 2019, 7:37 AM
kongyi added inline comments.
compiler-rt/lib/builtins/arm/fp_mode.c
25

Added check for __ARM_FP. Fallback to default rounding if VFP is not available.

efriedma added inline comments.Jan 28 2019, 4:49 PM
compiler-rt/lib/builtins/arm/fp_mode.c
25

I'm a little concerned this will do something unexpected on soft-float ARM Linux (which is usually built for a target without VFP support, but often runs on a chip that actually supports it). I guess always disabling other rounding modes is more intuitive than enabling them conditionally, though; should be fine.

The mix of soft-floating point and hard-floating point (with soft-float interface) can happen on embedded systems code as well. The solution Arm's proprietary toolchain had to this problem is for the linker to select libraries based on build-attibutes. It would detect the rounding mode build attribute and select the appropriate library variant, but that isn't available to us here. A colleague suggested an easily available customisation point that someone that had a local need for a different rounding mode could implement.

compiler-rt/lib/builtins/arm/fp_mode.c
25

In this situation I'm not sure we can do much better in generic compiler-rt. If we thought it would be necessary to allow change from FE_TONEAREST one possibility would be to replace the return FE_TONEAREST in

#else
  return FE_TONEAREST;
#endif

With a call to a weak definition of another function returning FE_TONEAREST . A user that cared sufficiently about the rounding mode could then reimplement this function with the rounding mode of their choice. This would be a bit easier than adding definitions of both fe_raise_inexact and fe_getround but not much.

kongyi updated this revision to Diff 184739.Feb 1 2019, 6:44 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 1 2019, 6:44 AM
kongyi updated this revision to Diff 184745.Feb 1 2019, 7:02 AM
kongyi marked an inline comment as done.
kongyi updated this revision to Diff 189951.Mar 8 2019, 3:29 PM

The mix of soft-floating point and hard-floating point (with soft-float interface) can happen on embedded systems code as well. The solution Arm's proprietary toolchain had to this problem is for the linker to select libraries based on build-attibutes. It would detect the rounding mode build attribute and select the appropriate library variant, but that isn't available to us here. A colleague suggested an easily available customisation point that someone that had a local need for a different rounding mode could implement.

I'm happy with Yi's latest update. Do we have anyone left with any objections? If there aren't any I'm inclined to approve.

efriedma accepted this revision.Mar 25 2019, 2:05 PM

My comments have been addressed, I think. Looks fine.

This revision is now accepted and ready to land.Mar 25 2019, 2:05 PM
This revision was automatically updated to reflect the committed changes.