[16] RFR(S): 8251525: AARCH64: Faster Math.signum(fp)

[Vladimir Ivanov]

> http://cr.openjdk.java.net/~dchuyko/8251525/webrev.01/

Changes in shared code look good.

Best regards,
Vladimir Ivanov

> On 8/13/20 2:51 PM, Vladimir Ivanov wrote:
>> Hi Dmitry,
>>
>> Some comments on shared code changes:
>>
>> src/hotspot/share/opto/library_call.cpp:
>>
>> +  case vmIntrinsics::_dsignum:
>> +    return UseSignumIntrinsic && 
>> (Matcher::match_rule_supported(Op_SignumD) ? inline_double_math(id) : 
>> false);
>>
>> There's no need in repeating UseSignumIntrinsic and 
>> (Matcher::match_rule_supported(Op_SignumD) checks.
>> C2Compiler::is_intrinsic_supported() already covers taht.
>>
>>
>> src/hotspot/share/opto/signum.hpp:
>>
>>   32 class SignumNode : public Node {
>>   33 public:
>>   34   SignumNode(Node* in) : Node(0, in) {}
>>   35   virtual int Opcode() const;
>>   36   virtual const Type *bottom_type() const { return NULL; }
>>   37   virtual uint ideal_reg() const { return Op_RegD; }
>>   38 };
>>
>> Any particular reason to keep SignumNode? I don't see any and would 
>> just drop it.
>>
>> Also, having a dedicated header file just for a couple of nodes with 
>> trivial implementations looks like an overkill. As an alternative 
>> location, intrinsicnode.cpp should be a better option.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 13.08.2020 14:04, Dmitry Chuyko wrote:
>>> Hello,
>>>
>>> Please review a faster version of Math.signum() for AArch64.
>>>
>>> Two new intrinsics (double and float) are introduced in general code, 
>>> with appropriate new nodes. New JTreg test is added to cover the 
>>> intrinsic case (enabled only for aarch64).
>>>
>>> AArch64 implementation uses FACGT (compare abslute fp values) and BSL 
>>> (fp bit selection) to avoid branches and moves to non-fp registers 
>>> and back.
>>>
>>> Performance results show ~30% better time in the benchmark with a 
>>> black hole [1] on Cortex. E.g. on random numbers 4.8 ns/op --> 3.5 
>>> ns/op, overhead is 2.9 ns/op.
>>>
>>> rfe: https://bugs.openjdk.java.net/browse/JDK-8251525
>>> webrev: http://cr.openjdk.java.net/~dchuyko/8251525/webrev.00/
>>> testing: jck, jtreg including new dedicated test
>>>
>>> -Dmitry
>>>
>>> [1] https://cr.openjdk.java.net/~dchuyko/8249198/DoubleSignum.java
>>>