Page MenuHomePhabricator

[PowerPC] Exploit VSX rounding instrs for rint
ClosedPublic

Authored by qiucf on Jan 14 2020, 1:28 AM.

Details

Summary

This patch exploits native VSX rounding instr, x(v|s)r(d|s)pic, which does rounding using current rounding mode.

According to C standard library document, rint may raise INEXACT exception while nearbyint won't.

Diff Detail

Event Timeline

qiucf created this revision.Jan 14 2020, 1:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 1:28 AM

Unit tests: pass. 61803 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

lkail added a subscriber: lkail.Mon, Feb 10, 9:04 PM

Correct me if I'm wrong. rint(x) returns x if x is an NaN. However, intruction like XSRDPIC may turn SNAN to QNAN. Does it matter?

kpn added a comment.Tue, Feb 11, 9:18 AM

Correct me if I'm wrong. rint(x) returns x if x is an NaN. However, intruction like XSRDPIC may turn SNAN to QNAN. Does it matter?

It sounds like a problem. I don't have an IEEE reference handy, but I'm told POSIX forbids it.

I'm sorry I'm not a PowerPC guy, I'm more of a SystemZ guy, so I can't sign off on these changes. The PPCISelLowering.cpp changes look reasonable.

qiucf added a comment.Wed, Feb 12, 7:34 PM

Correct me if I'm wrong. rint(x) returns x if x is an NaN. However, intruction like XSRDPIC may turn SNAN to QNAN. Does it matter?

I didn't find the language standard ensuring this (only 'return NaN for NaN'). It seems Glibc's rint would also return a qNaN for sNaN, while some other implementations keeps it. Is that specified by any doc/spec? Thanks.

lkail added a comment.Wed, Feb 12, 7:47 PM

Is that specified by any doc/spec? Thanks.

I found it in man rint on linux, which writes

RETURN VALUE
       These functions return the rounded integer value.

       If x is integral, +0, -0, NaN, or infinite, x itself is returned.
qiucf added a comment.Thu, Feb 13, 1:30 AM

@lkail You can compile this simple program and test. I got following results on Linux with GCC (call for rint):

7FF8000000000000 7FF4000000000000 7FFC000000000000
0 1 0

I believe the conversion of SNaN to QNaN is expected here. Note that the (current) C standard does not mention support signaling NaNs at all, and does not really ever mention them. This is planned to be fixed with the upcoming C2x version, which explicitly states that "rint" is supposed to implement the IEEE-754 "roundToIntegerExact" function. And that function, like most general functions defined by IEEE-754, is indeed defined to return a QNaN when the input is a SNaN.

So the current behavior already implements C2x correctly, and is a valid extension to C11, so this should be fine.

lkail accepted this revision.Thu, Feb 13, 2:10 AM

I believe the conversion of SNaN to QNaN is expected here. Note that the (current) C standard does not mention support signaling NaNs at all, and does not really ever mention them. This is planned to be fixed with the upcoming C2x version, which explicitly states that "rint" is supposed to implement the IEEE-754 "roundToIntegerExact" function. And that function, like most general functions defined by IEEE-754, is indeed defined to return a QNaN when the input is a SNaN.

Thanks for the information.

Since my concern has been resolved, this patch LGTM. Thanks for exploiting these instructions.

This revision is now accepted and ready to land.Thu, Feb 13, 2:10 AM
This revision was automatically updated to reflect the committed changes.