RFR(M): 8027754: Enable loop optimizations for loops with MathExact inside

[Igor Veresov]

On Feb 4, 2014, at 2:00 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:

> Igor,
> 
> what about DOMAIN?

Sounds ok.

igor


> Also looking for an additional Reviewer.
> 
> Thanks
> /R
> 
> 
> On 01/31, Igor Veresov wrote:
>> library_call.cpp:
>> 
>> You forgot to remove this:
>> 208   template <typename OverflowOp>
>> 209   bool inline_math_overflow(bool is_unary);
>> 
>> type.hpp:
>> 
>> Come to think of it again bottom() is not really a bottom. I can’t come up with good name for it. Perhaps SIGNED ?
>> And define it as static constant:
>> static const TypeInt *SIGNED = INT; 
>> and 
>> static const TypeLong *SIGNED = LONG;
>> to follow the existing convention?
>> 
>> Otherwise looks excellent.
>> 
>> igor
>> 
>> 
>> On Jan 31, 2014, at 10:22 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>> 
>>> 
>>> Seem to have pasted the wrong link, try this one:
>>> 
>>> http://cr.openjdk.java.net/~rbackman/8027754.2/
>>> 
>>> /R
>>> 
>>> On 01/31, Christian Thalinger wrote:
>>>> 
>>>> On Jan 31, 2014, at 3:44 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>>>> 
>>>>> Igor,
>>>>> 
>>>>> I changed the calls in library_call.cpp according to your suggestions.
>>>>> The same with mathexactnode.cpp. Also did some additional cleanup there.
>>>>> 
>>>>> Made will_overflow and can_overflow pure virtual in the base classes.
>>>>> 
>>>>> The sub method is required as OverflowNode inherits from CmpNode.
>>>>> 
>>>>> Updated webrev: http://cr.openjdk.java.net/~rbackman/8028997.2/
>>>> 
>>>> That webrev seems to not have all changes.
>>>> 
>>>>> 
>>>>> Thanks
>>>>> /R
>>>>> 
>>>>> On 01/30, Igor Veresov wrote:
>>>>>> Hi Rickard,
>>>>>> 
>>>>>> In library_call.cpp:
>>>>>> 
>>>>>> May be it’s just me but I’d get rid of bool LibraryCallKit::inline_math_overflow(bool is_unary) and reference the arguments explicitly. Otherwise it feels like too much depth to follow through when reading the code(since we keep specialized functions like inline_math_addExactI() anyway).
>>>>>> 
>>>>>> Something like:
>>>>>> bool LibraryCallKit::inline_math_addExactI(bool is_increment) {
>>>>>> return inline_math_overflow<OverflowAddINode>(argument(0), is_increment ? intcon(1) : argument(1));
>>>>>> }
>>>>>> Which will also allow you to avoid adding the IsLong enum.
>>>>>> 
>>>>>> In mathexactnode.cpp:
>>>>>> 
>>>>>> I’d move the AddHelper, SubHelper, MulHelper to the cpp file and reference them directly in a more verbose way.
>>>>>> Something like:
>>>>>> bool OverflowAddINode::will_overflow(jint v1, jint v2) const {
>>>>>>  return AddHelper<OverflowINode>::will_overflow(v1, v2);
>>>>>> }
>>>>>> Otherwise the reader has to go and find out what OverflowHelper means in a particular context.  This will also reduce the amount of implementation specifics in the hpp. 
>>>>>> 
>>>>>> 
>>>>>> Is there a particular reason why you define will_overflow() and can_overflow() to do ShoundNotReachHere() is the base classes instead of making them pure virtual and let the compiler make sure that you define them? I guess there is an expectation that there are going to be overflow nodes that don’t redefine these methods but are there really going to be any?
>>>>>> 
>>>>>> You also define const Type* sub(const Type* t1, const Type* t2) const as ShouldNotReachHere() and never redefine it. What are your plans for it?
>>>>>> 
>>>>>> igor
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On Jan 30, 2014, at 5:10 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>>>>>> 
>>>>>>> Igor,
>>>>>>> 
>>>>>>> thanks for looking at this and the suggestions. I added some templates
>>>>>>> to reduce the amount of code.
>>>>>>> 
>>>>>>> Webrev: http://cr.openjdk.java.net/~rbackman/8027754.1/
>>>>>>> 
>>>>>>> Thanks
>>>>>>> /R
>>>>>>> 
>>>>>>> On 01/23, Igor Veresov wrote:
>>>>>>>> Nice!
>>>>>>>> 
>>>>>>>> In library_call.cpp:
>>>>>>>> 
>>>>>>>> Could the LibaryCallKit::inline_math*() family of functions be factored with templates to shave a few lines? There is quite a lot of common code.
>>>>>>>> I think the overflow idiom insertion can be something like:
>>>>>>>> 
>>>>>>>> template<typename OperationNodeType, template OverflowNodeType>
>>>>>>>> void LibraryCallKit::inline_overflow(Node* arg1, Node* arg2) {
>>>>>>>> Node* op = _gvn.transform(new(C) OperationNodeType(arg1, arg2)); 
>>>>>>>> Node* of = _gvn.transform(new(C) OverflowNodeType(arg1, arg2));
>>>>>>>> inline_math_mathExact(op, of);
>>>>>>>> }
>>>>>>>> 
>>>>>>>> In mathexactnode.cpp:
>>>>>>>> You’ve already commoned many things up by introducing OverflowINode and OverflowLNode in hierarchy. 
>>>>>>>> But it feels like some of the code there could factored up as well using template helpers. In many cases the code looks exactly the same for ints and longs, differing only in some types.
>>>>>>>> 
>>>>>>>> 
>>>>>>>> igor
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On Jan 23, 2014, at 3:27 AM, Rickard Bäckman <rickard.backman at oracle.com> wrote:
>>>>>>>> 
>>>>>>>>> Hi all,
>>>>>>>>> 
>>>>>>>>> this change is going to 9 (and backporting to 8u20). Can I please have
>>>>>>>>> this change reviewed?
>>>>>>>>> 
>>>>>>>>> The implementation of different j.l.Math.mathExact() didn't work very
>>>>>>>>> well with different optimizations because there was one node that
>>>>>>>>> generated both control and data. This change has a new implementation
>>>>>>>>> where each call to j.l.Math.mathExact() generates a Overflow node and a
>>>>>>>>> normal math operation node (in the integer add example: OverflowAddINode
>>>>>>>>> and a AddINode). The Overflow node is responsible for generating
>>>>>>>>> control.
>>>>>>>>> 
>>>>>>>>> In the end we generate assembly like:
>>>>>>>>> 
>>>>>>>>> mov rdx, rdi
>>>>>>>>> add rdx, rsi
>>>>>>>>> ...
>>>>>>>>> mov rax, rdi
>>>>>>>>> add rax, rsi
>>>>>>>>> jo <overflow label>
>>>>>>>>> 
>>>>>>>>> With one add instruction for the data and one for flags. Future
>>>>>>>>> improvements could be to try to match the Overflow and the math
>>>>>>>>> operation and remove one of them.
>>>>>>>>> 
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8027754
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rbackman/8027754/
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> /R
>>>>>> 
>>>> 
>>