RFR(L) 8026844: Various Math functions needs intrinsification

[Vladimir Kozlov]

> On Oct 23, 2013, at 12:45 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> Looks good to me.

May be not. I missed that you removed your changes in final graph reshape for phi node. You misinterpreted my comment. I said split through phi optimization should not be done. If phi come from loop code - it is different issue. I think you can keep your code for phi otherwise you may get problems,

Thanks,
Vladimir

> 
> Thanks,
> Vladimir
> 
>> On Oct 23, 2013, at 9:24 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>> 
>> Vladimir, thanks for taking time to look at this. See inline.
>> 
>>> On 10/23, Vladimir Kozlov wrote:
>>> Richard,
>>> 
>>> Node.hpp and corresponding constructors. You should specify id only for MathExact class since you only need query method for it (no need to specify it for subclasses). The number in DEFINE_CLASS_ID should be 4 (not 7) since the last one (for Membar) was 3.
>> 
>> Ok.
>> 
>>> 
>>> I do not understand your problem with shared nodes (matcher.cpp changes). Sharing means that separate Mach node will be generated (it will not be part of mega Mach node). So I do not see the problem. Is it load nodes you have problem with? Please, explain.
>> 
>> Removed. is_shared is also passed as an argument to match_into_reg. If
>> shared we will never put it into a register.
>> 
>>> About phi node. Look if it is created by spit_through_phi optimization. We should not do that because if addexact, for example, collapsed/folded on one branch how we get flag value from this branch.
>> 
>> The Phi was (part of) an induction variable in this case.
>> 
>> New webrev: http://cr.openjdk.java.net/~rbackman/8026844.2/
>> 
>> Thanks!
>> 
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>>> On Oct 22, 2013, at 9:56 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>>>> 
>>>> Christian,
>>>> 
>>>> new webrev: http://cr.openjdk.java.net/~rbackman/8026844.1/
>>>> The diff between webrevs: http://cr.openjdk.java.net/~rbackman/8026844.1.diff/
>>>> (I missed x86_32.ad when generating it, but everything else should be
>>>> there).
>>>> 
>>>> I removed the timeouts, they were remains of another attempt at writing
>>>> tests.
>>>> 
>>>> Thanks
>>>> 
>>>>> On 10/18, Christian Thalinger wrote:
>>>>> src/share/vm/opto/library_call.cpp:
>>>>> 
>>>>> +   Node* arg1 = argument(0); // type long
>>>>> +   // argument(1) == TOP
>>>>> +   Node* arg2 = argument(2); // type long
>>>>> +   // argument(4) == TOP
>>>>> 
>>>>> The "argument(4)" comment it wrong; should be 3.
>>>>> 
>>>>> +   Node* add = _gvn.transform( new(C) AddExactLNode(NULL, arg1, arg2) );
>>>>> 
>>>>> Remove the extra spaces.
>>>>> 
>>>>> src/share/vm/opto/mathexactnode.hpp:
>>>>> 
>>>>> +   MathExactNode(Node* ctrl, Node* n1);
>>>>>  MathExactNode(Node* ctrl, Node* in1, Node* in2);
>>>>> 
>>>>> Should "n1" be "in1"?  Or maybe just "n".
>>>>> 
>>>>> These have two copyright headers:
>>>>> 
>>>>> test/compiler/intrinsics/mathexact/IncExactITest.java
>>>>> test/compiler/intrinsics/mathexact/IncExactLTest.java
>>>>> 
>>>>> It seems all of the tests have a wrong bug number:
>>>>> 
>>>>> 26  * @bug 8024924
>>>>> 
>>>>> or
>>>>> 
>>>>> 26  * @bug 8025657
>>>>> 
>>>>> test/compiler/intrinsics/mathexact/MulExactIRepeatTest.java:
>>>>> 
>>>>> 28  * @compile MulExactIRepeatTest.java
>>>>> 
>>>>> That one doesn't compile Verify.java.  Please check all of the test files for these issue.
>>>>> 
>>>>> In some test files you have timeouts.  Will there be a timeout problem on slower embedded platforms?
>>>>> 
>>>>> The question is do we really need individual test files for these?  I don't object just mentioning it.
>>>>> 
>>>>>> On Oct 18, 2013, at 2:34 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>>>>>> 
>>>>>> Hi, can I get reviews for the following change:
>>>>>> 
>>>>>> This change creates intrinsics for addExact(long, long), subtractExact,
>>>>>> negateExact, incrementExact, decrementExact and multiplyExact.
>>>>>> 
>>>>>> The intrinsics are only added on x86 and only 64-bit has the intrinsics
>>>>>> for the long-versions. (32-bit only has int-versions).
>>>>>> 
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8026844
>>>>>> Webrev: http://cr.openjdk.java.net/~rbackman/8026844/
>>>>>> 
>>>>>> Thanks
>>>>>> /R
>>>> /R
>> /R