RFR(S): 8074869: C2 code generator can replace -0.0f with +0.0f on Linux
zoltan.majo at oracle.com
Fri Mar 13 18:25:39 UTC 2015
thank you for testing this patch and also for providing the test!
On 03/13/2015 04:54 PM, Volker Simonis wrote:
> Hi Zoltan,
> I've tested the change on Linux/ppc64 and AIX and it works fine.
> However I don't really see the bug which you pretend to fix with this change.
> All the Linux architectures which are in the OpenJDK use the following
> predicate to check for +0.0:
> predicate((n->getf() == 0) &&
> (fpclassify(n->getf()) == FP_ZERO) && (signbit(n->getf()) == 0));
> In particular they explicitly check the sign bit of the float/double
> argument which should ensure correct operation.
That is correct. However, the current code explicitly makes the
assumption FP_ZERO on Linux is equivalent to Solaris's FP_PZERO. That is
a wrong assumption and it can be misleading. This patch changes that
part of the code. Moreover, with the new code we use the same cast-based
checks on all architectures.
> I wrote a small regression test (added to the new webrev for your
> convenience ) which proves the correct operation with the current
> code. If I remove the "signbit" check the regression test will fail.
> Nevertheless I think your change is good because it is a nice cleanup
> and simplification. So please go ahead and push it.
Thank you for your feedback!
> PS: I've also removed some trailing whitespace from your original change
>  http://cr.openjdk.java.net/~simonis/webrevs/2015/8074869/
> On Thu, Mar 12, 2015 at 1:43 PM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
>> please review the following small patch.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8074869
>> Problem: On Linux, the C2 code generator can replace the value -0.0f with
>> +0.0f (and also the value -0.0d with +0.0d). The reason is that in some *.ad
>> files both the value -0.0f and +0.0f is treated as being +0.0f and can
>> therefore be replaced with an immediate +0.0f embedded into an instruction.
>> For example, in the sparc.ad file, the 'fpclass' function is used to decide
>> if a float node's content is +0.0:
>> predicate((n->getf() == 0) && (fpclass(n->getf()) == FP_PZERO));
>> On Solaris, 'fpclass' returns FP_PZERO if the parameter is +0.0f and
>> FP_NZERO if the parameter is -0.0f. As a result, +0.0f and -0.0f are
>> distinguished by the compiler.
>> On Linux, however, 'fpclass' is not available and therefore 'fpclassify' is
>> used. 'fpclassify' does not distinguish between ±0.0f, it returns FP_ZERO
>> for both +0.0f and -0.0f.
>> Solution: Instead of 'fpclass', use cast float->int and double->long to
>> check if value is +0.0f and +0.0d, respectively. This logic is already use
>> on some architectures, for example on x86_32 and on x86_64.
>> As 'fpclass' is not used any more, remove its declarations from
>> globalDefintions_*. The include of ieeefp.h must be kept as we rely on some
>> other functionality from this header on solaris.
>> Webrev: http://cr.openjdk.java.net/~zmajo/8074869/webrev.00/
>> - JPRT testing on all supported platforms (does *not* include aarch64 and
>> - manual testing on aarch64:
>> All DaCapo benchmarks with the small input size. I used the default JVM
>> flags and tested the VM w/ and w/o the fix. All benchmarks pass except
>> eclipse. For eclipse, the same Java-level failure appears both w/ and w/o
>> the fix, so I think the problem with eclipse is not due to these changes.
>> I also tested with the "-Xcomp -XX:-TieredCompilation -server" flags.
>> Eclipse fails in this case as well. Additionally, tradebeans and tradesoap
>> fail with a Java-level failure. As the failure happens also with both builds
>> (w/ and w/o the fix), I don't think the problem is caused by these changes
>> - no testing on ppc64: I don't have access to a ppc64 machine. Could
>> somebody with access to a ppc64 machine please build and test the VM with
>> this patch (and then maybe confirm that it works)?
>> Thank you and best regards,
More information about the hotspot-compiler-dev