This is an archive of the discontinued LLVM Phabricator instance.

Stop traping on sNaN in __builtin_isnan
ClosedPublic

Authored by thopre on Feb 3 2021, 5:41 AM.

Details

Summary

__builtin_isnan currently generates a floating-point compare operation
which triggers a trap when faced with a signaling NaN in StrictFP mode.
This commit uses integer operations instead to not generate any trap in
such a case.

Diff Detail

Event Timeline

thopre created this revision.Feb 3 2021, 5:41 AM
thopre requested review of this revision.Feb 3 2021, 5:41 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 3 2021, 5:41 AM
mibintc added inline comments.Feb 3 2021, 8:02 AM
clang/lib/CodeGen/CGBuiltin.cpp
2988

What about all the other FIXME related to strictfp, are we going to pick them off piecemeal? It would be nice to have a holistic approach, it would be more clear, and less duplicated code. For example, presumably the test at line 2991 is common.

3001

Are you using a reference (e.g. existing implementation) for this rewrite, or is this invention? If a reference can you please tell me what it is. The expression you have written here doesn't match the codegen below. I don't see the comparison to zero. Can you provide full parenthesization--the compare to zero is lower than subtract?

3010

compared to the comment above at line 3001, lhs and rhs are swapped in the sub

3011

the comment at line 3001 doesn't show the lshr or the compare to zero

clang/test/CodeGen/X86/strictfp_builtins.c
2

Why did you put long double into this new test case instead of putting it with the float and double in strictfp_builtins?

thopre marked 2 inline comments as done.Feb 3 2021, 8:11 AM
thopre added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
2988

I do want to do isinf and other similar builtin but I would prefer to have a review of this to see if there's anything wrong with my approach before I work on other builtins. Once I do I'll factor things out as needed.

3010

My bad, the comment needs to be fixed.

3011

This logical right shift will move the sign bit to bit0 and set all other bits to 0. The result will be 1 (true) if sign bit is set (sub is negative) or 0 (false) otherwise. That matches the comment. Would you like me to add a comment here to make it more explicit?

clang/test/CodeGen/X86/strictfp_builtins.c
2

long double is implemented differently for each target. x86 uses an 80 bit extended precision format, AFAIK AArch64 uses a 128bit IEEE format, etc.

thopre marked 2 inline comments as done.Feb 3 2021, 8:18 AM
thopre added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
2995

I'm not too sure if that's enough to check isIEEE here. x86 extended precision behaves a bit different to IEEE single and double precision, esp. wrt. infinite and NaN. I've decided to ignore NaN values that are deprecated since the 8087.

3001

I've noticed glibc isnan implementation does not trigger a trap so I've looked at the assembly it generates for the float case. This is the generalized version for double and long double as well.

thopre updated this revision to Diff 321107.Feb 3 2021, 8:48 AM
thopre marked 2 inline comments as done.
  • Fix order of sub operands in comments and use fully parenthesized expression
  • Explain how LShr is equivalent to < 0
mibintc added inline comments.Feb 3 2021, 12:33 PM
clang/lib/CodeGen/CGBuiltin.cpp
3011

No, that's OK, you don't need to do that. thanks. I'd like to see what other reviewers say, thanks for working on this!

kpn added a comment.Feb 4 2021, 11:00 AM

This looks like a definite step forward. Thank you!

Can you do an AArch64 test case showing long double? Right now there's no 128-bit test case.

Should we add tests for mlong-double-64, -80, -128?

kpn added a comment.Feb 4 2021, 11:24 AM

Should we add tests for mlong-double-64, -80, -128?

Assuming Ty->getScalarSizeInBits() returns 64, 80, or 128 in those cases, I think it's good enough to just add an AArch64 (or some other 128-bit long double host) and call it a day. We're testing isnan(), not that command line option, after all.

thopre updated this revision to Diff 321554.Feb 4 2021, 1:16 PM

Add AArch64 testcase and rebase

clang/test/CodeGen/aarch64-strictfp-builtins.c
1

AArch64 is not StrictFP enabled so we need this option.

kpn accepted this revision.Feb 5 2021, 9:13 AM

LGTM

This revision is now accepted and ready to land.Feb 5 2021, 9:13 AM
This revision was landed with ongoing or failed builds.Feb 5 2021, 10:28 AM
This revision was automatically updated to reflect the committed changes.