This is an archive of the discontinued LLVM Phabricator instance.

[builtins] Raise FE_INVALID/FE_DIVBYZERO for 128-bit float math functions
Needs ReviewPublic

Authored by arichardson on Mar 10 2021, 3:59 AM.

Details

Summary

I have been trying to get the FreeBSD libm testsuite to pass on AArch64
and it expects certain long double math functions to raise FE_INVALID for
bad inputs. However, the compiler-rt soft-float builtins do not raise
FE_INVALID for cases such as inf-inf, 0/0 etc. so the tests fail.

This patch extends the existing support for raising FE_INEXACT to also
handle FE_INVALID/FE_DIVBYZERO for most cases listed in IEEE-754-2019 7.2
and 7.3. I am not a floating-point expert so I may have missed some cases,
or might have got something wrong but this change allows most of the
FreeBSD libm long double tests to pass on AArch64.

Diff Detail

Event Timeline

arichardson created this revision.Mar 10 2021, 3:59 AM
arichardson requested review of this revision.Mar 10 2021, 3:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 3:59 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

From another non-floating point expert, this looks reasonable to me. I'll see if I can find anyone internally that knows more and would like to take a look.

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

Please could the comment be moved to before ARM_INEXACT. It can be worked out from the 0x1000 what it is referring to, but I think having it above the value would avoid suspicion of ARM_INVALID and ARM_DIVBYZERO.

I'm with you that it looks wrong. From my reading of the Arm ARM 0x1000 (12 IXE) enables hardware trapping of inexact rounding (if hardware supports it), which would be unlikely to be useful given that __fe_raise_inexact is called when adding/subtracting floating point in software.

From another non-floating point expert, this looks reasonable to me. I'll see if I can find anyone internally that knows more and would like to take a look.

Thanks!

I'll also split the ARM_INEXACT change to a separate review.

arichardson marked an inline comment as done.

One small concern related to the tests on a target that doesn't implement __fe_raise_invalid and friends.

compiler-rt/lib/builtins/fp_add_impl.inc
29

Something of this form is repeated 6 times (check for isNaN then raise invalid). I'm wondering if this could be hoisted into an inline function. It might make the code easier to follow.

It looks marginal so I can easily see it not being worthwhile, but maybe worth an attempt to see what it looks like.

compiler-rt/test/builtins/Unit/addtf3_test.c
20

What would happen if we ran the test on a target that just returned 0 for __fe_raise_invalid? I can't immediately see how actualExc == expectedExc in that case. I could be missing something though.

arichardson added inline comments.Mar 12 2021, 3:07 AM
compiler-rt/lib/builtins/fp_add_impl.inc
29

Yes I think that would be more readable, and the compiler should be able to fold the redundant checks anyway.
Will try and update on Monday.

compiler-rt/test/builtins/Unit/addtf3_test.c
20

Good point, this test would fail then. Maybe best to change it to something like

if (CRT_SETS_FP_EXC && actualExc != expectedExc)

and have that be pre-defined by lit depending on the architecture.

  • Don't check fp exception flags if compiler-rt doesn't set them.

Thanks for the update. Test changes look good to me. Just wanted to check to see if you had any changes planned for lifting some of the common tests into a function? Or if that can be left to another patch?

Thanks for the update. Test changes look good to me. Just wanted to check to see if you had any changes planned for lifting some of the common tests into a function? Or if that can be left to another patch?

Ah sorry I forgot about that comment. Currently busy merging upstream LLVM into our fork, but I can make that change once I complete the merge.

Apologies for the long delay just rediscovered this due to test failures.

Rebased on latest main, dropping dependency on my unmerged builtins changes.
I have not factored out the snan checks to a helper function, will try to do that if I find the time.

Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2023, 5:36 PM
Herald added a subscriber: Enna1. · View Herald Transcript

Add a isSNaN helper function

Thanks for the update. I'll try and get someone from the team with more of a background in floating point to take a look. If not I'll do my best to check it.

Also handle RISC-V (tested with GCC+user-mode QEMU)

arichardson added inline comments.Jun 28 2023, 9:50 AM
compiler-rt/test/builtins/Unit/multf3_test.c
12

Hmm looks like this should probably return 0 to skip the test or we add an XFAIL: for x86 since it incorrectly defines multf.

simon_tatham added inline comments.
compiler-rt/lib/builtins/fp_fixint_impl.inc
31

Nit: shouldn't this also raise the invalid exception in cases where the input exponent is in range but the value rounds up to overflow? E.g. converting 0x7fffffff.ffff to int32_t.

compiler-rt/lib/builtins/fp_fixuint_impl.inc
25

If the number is negative, shouldn't we raise invalid? At least if it's <= -0.5 so that it rounds down to a negative integer instead of up to 0.

29

Another 'round up to overflow' case here, e.g. 0xffffffff.ffff → uint32_t should raise invalid, and I don't think it will with this code.

simon_tatham added inline comments.Jul 3 2023, 3:31 AM
compiler-rt/lib/builtins/fp_fixint_impl.inc
31

Oh, hang on – maybe I'm thinking of the wrong rounding mode. This is unconditionally doing round-towards-zero, isn't it? So this code is just fine. Sorry for the noise.

compiler-rt/lib/builtins/fp_fixuint_impl.inc
25

My mistake: if we're rounding towards zero, then x <= -1 is the criterion for raising invalid, not x <= -0.5.

(And even in round-to-nearest/even it should have been x < -0.5, ahem.)

29

I retract this complaint for the same reason as the signed integer one.