[aarch64-port-dev ] [16] RFR(S): 8251525: AARCH64: Faster Math.signum(fp)

[Vladimir Ivanov]

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

Overall, shared code changes look fine.

src/hotspot/share/opto/intrinsicnode.cpp:

+//------------------------------CopySign-----------------------------------------
+CopySignDNode* CopySignDNode::make(PhaseGVN& gvn, Node* in1, Node* in2) {
+  return new CopySignDNode(in1, in2, gvn.makecon(TypeD::ZERO));
+}
+
+//------------------------------Signum-------------------------------------------
+SignumDNode* SignumDNode::make(PhaseGVN& gvn, Node* in) {
+  return new SignumDNode(in, gvn.makecon(TypeD::ZERO), 
gvn.makecon(TypeD::ONE));
+}
+
+SignumFNode* SignumFNode::make(PhaseGVN& gvn, Node* in) {
+  return new SignumFNode(in, gvn.makecon(TypeF::ZERO), 
gvn.makecon(TypeF::ONE));
+}

Putting auxiliary constants on ideal nodes doesn't look right 
(platform-specific implementation details leaking into shared code), but 
possible alternatives are much uglier.

C2 doesn't rematerialize FP constants on AArch64, so code quality 
benefits from sharing ConF/ConD nodes.

Introducing "constants as TEMPs" support in Matcher (insert constants as 
additional inputs during matching) seems the best option, but it is not 
implemented yet. (Something to consider as a separate enhancement.)

Best regards,
Vladimir Ivanov

> On 9/1/20 6:48 PM, Andrew Haley wrote:
>> On 31/08/2020 15:28, Dmitry Chuyko wrote:
>>> Here is another version of intrinsics. It is an extension of webrev.03.
>>> Additional thing is that constants 0 and 1 that are used internally by
>>> intrinics are constructed as nodes. This is somehow similar to what is
>>> done for passing pointers to tables.
>>>
>>> webrev: http://cr.openjdk.java.net/~dchuyko/8251525/webrev.04/
>>> results:
>>> http://cr.openjdk.java.net/~dchuyko/8251525/webrev.04/benchmarks/signum-facgt_ir-copysign.ods 
>>>
>> Hi,
>>
>> Thank you. That certainly looks better.
>>
>> It's unfortunate that signum doesn't help in all cases, but I'm happy
>> that we have something positive in general. Certainly the code looks
>> nice. I'm still rather baffled that an intrinsification of copySign
>> actually makes things much worse on blackhole on Neoverse N1, but it
>> doesn't really matter because the copySign intrinsic isn't enabled by
>> default. So please go ahead with this version.
>>
>> Having said all of that, it's a fairly minor improvement for some
>> considerable complexity. And it depends terribly on the
>> micorarchitecture of a particular part, albeit an important one.
>>