RFR(M) 8024924: Intrinsify java.lang.Math.addExact
Christian Thalinger
christian.thalinger at oracle.com
Wed Sep 25 12:51:16 PDT 2013
On Sep 23, 2013, at 5:43 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
> Christian,
>
> thanks for the comments. See inline.
>
> On Sep 21, 2013, at 1:49 AM, Christian Thalinger wrote:
>
>> You should change the bug's summary to mention that this only implements the integer version of addExact; there is also a long version. Maybe file an RFE for the long version.
>
> Ok.
>
>>
>> src/share/vm/opto/subnode.hpp:
>>
>> ! enum mask { eq = 0, ne = 4, le = 5, ge = 7, lt = 3, gt = 1, of = 2, nof = 6, illegal = 8 };
>>
>> More a remark than a serious review comment but I'd like "o" and "no" more than "of" and "nof". Some places in HotSpot use "nof" as "number of" (I don't like that either but that's how it is).
>
> I would rather not shorten it further. I could agree on expanding it to overflow, no_overflow or not_overflow though.
> Thoughts?
I'm all for making it more clearer. All this shortening doesn't buy us anything anyway.
>
>>
>> src/share/vm/opto/mathexactnode.cpp:
>>
>> Thanks for putting this in a new file!
>>
>> 66 if (bolnode->_test._test == BoolTest::of) {
>> 67 // if the check is for overflow - never taken
>> 68 igvn->replace_node(bolnode, phase->intcon(0));
>> 69 } else if (bolnode->_test._test == BoolTest::nof) {
>> 70 // if the check is for not overflow - always taken
>> 71 igvn->replace_node(bolnode, phase->intcon(1));
>> 72 }
>>
>> Can we read bolnode->_test._test once and switch on it? With a default case that has a fatal?
>
> Ok.
>
>>
>> 96 } else if (type1 == TypeInt::ZERO) {
>> 97 Node* add_result = new (phase->C) AddINode(arg1, arg2);
>> 98 return no_overflow(phase, add_result);
>> 99 } else if (type2 == TypeInt::ZERO) {
>> 100 Node* add_result = new (phase->C) AddINode(arg1, arg2);
>> 101 return no_overflow(phase, add_result);
>> 102
>> 103 } else if (type2->singleton()) {
>> 104 return NULL; // no change
>> 105 } else if (type1->singleton()) {
>> 106 // Make it x + Constant
>> 107 swap_edges(1, 2);
>> 108 return this;
>> 109 } else if (arg2->is_Load()) {
>> 110 return NULL; // no change
>> 111 } else if (arg1->is_Load()) {
>> 112 // Make it x + Load
>> 113 swap_edges(1, 2);
>> 114 return this;
>> 115 } else if (arg1->_idx > arg2->_idx) {
>> 116 // Sort the edges
>> 117 swap_edges(1, 2);
>> 118 return this;
>> 119 }
>>
>> This stuff is hard to read. Since all of these ifs return we could remove the else. Also put a return NULL into the first if of that method.
>
> Ok.
>
>>
>> Otherwise this looks good.
>
> Thanks
> /R
>
>>
>> On Sep 20, 2013, at 12:58 PM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>>
>>> Christian, review please?
>>>
>>> /R
>>>
>>>> 20 sep 2013 kl. 17:40 skrev Vladimir Kozlov <vladimir.kozlov at oracle.com>:
>>>>
>>>> It is good. Ask Christian and Igor to get second review.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>> On 9/20/13 4:34 AM, Rickard Bäckman wrote:
>>>>> Here is an updated webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~rbackman/8024924.2/
>>>>>
>>>>> Moved the tests to compiler/intrinsics/mathexact
>>>>> and moved the new nodes to mathexactnode.[ch]pp
>>>>>
>>>>> Thanks
>>>>> /R
>>>>>
>>>>>> On Sep 19, 2013, at 9:36 PM, Rickard Bäckman wrote:
>>>>>>
>>>>>> Vladimir,
>>>>>>
>>>>>> see inline.
>>>>>>
>>>>>>> On Sep 19, 2013, at 9:29 PM, Vladimir Kozlov wrote:
>>>>>>>
>>>>>>>> On 9/19/13 11:46 AM, Rickard Bäckman wrote:
>>>>>>>> Vladimir,
>>>>>>>>
>>>>>>>> thanks for the review. I'll update the code accordingly.
>>>>>>>>
>>>>>>>>> On Sep 19, 2013, at 8:15 PM, Vladimir Kozlov wrote:
>>>>>>>>>
>>>>>>>>> Rickard,
>>>>>>>>>
>>>>>>>>> You need to provide more description for public and include the link to the bug report which now is accessible from outside:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8024924
>>>>>>>>
>>>>>>>> Sorry missed that one.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> When result of MathExact is not used we should replace it with top to remove uncommon trap.
>>>>>>>>
>>>>>>>> You are saying that if the ProjNode (result) doesn't have an out we should replace the MathExact with top() right?
>>>>>>>
>>>>>>> Yes, if it is IGVN. Make sure FlagsProjNode handles such case (top input).
>>>>>>>
>>>>>>> ProjNode could be gone too, eliminated by remove_globally_dead_node(). You may need to add MathExact node check to has_special_unique_user() so MathExact will be put on _worklist in such case:
>>>>>>>
>>>>>>> } else if (in->outcnt() == 1 &&
>>>>>>> in->has_special_unique_user()) {
>>>>>>> _worklist.push(in->unique_out());
>>>>>>>
>>>>>>> Note, usually we return top from ::Value() method.
>>>>>>
>>>>>> But can I really remove the uncommon trap if the use of the value goes dead?
>>>>>> I mean:
>>>>>>
>>>>>> addExact(big value, big value)
>>>>>>
>>>>>> that doesn't have a use of the sum. But it would throw an exception.
>>>>>> If we remove the uncommon trap… we would no longer throw an exception.
>>>>>>
>>>>>> /R
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> I think it would be better to create new mathexactnode.?pp files for new mathexact nodes. Our *node.?pp files become too fat.
>>>>>>>>
>>>>>>>> Good idea.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> In MultiNode::proj_out(proj) check is_FlagsProj() first since is_Proj() is also true for it (since it is subclass).
>>>>>>>>
>>>>>>>> No it doesn't. is_Proj() is controlled by the macros in nodes.hpp. FlagsProjNode is a child of CmpNode there and can't be a member of both. I hit the assert in proj_out()
>>>>>>>
>>>>>>> You are right.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Next test file misses Copyright header:
>>>>>>>>>
>>>>>>>>> test/compiler/8024924/Verify.java
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> We are moving to creation of subcomponent subdirs for new tests. Could you move that into test/compiler/intrinsic/8024924 ?
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>> /R
>>>>>>>>
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>>> On 9/18/13 11:55 AM, Rickard Bäckman wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> can I please have reviews for this change.
>>>>>>>>>> Jump on overflow is implemented on x86 32&64-bit.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~rbackman/8024924.1/
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> /R
>>>>>
>>
>
More information about the hotspot-compiler-dev
mailing list