RFR(M): 8173470: [C2] Mask shift operands in ideal graph.

Wed Feb 22 19:09:58 UTC 2017

Tests passed. I am pushing this fix.

Thanks,
Vladimir

On 2/22/17 12:47 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
> that's great, thank you!
> Our tests are fine, also with my recent edits.
>
> Best regards,
>   Goetz.
>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Mittwoch, 22. Februar 2017 00:59
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-compiler-
>> dev at openjdk.java.net
>> Subject: Re: RFR(M): 8173470: [C2] Mask shift operands in ideal graph.
>>
>> Good. I will sponsor it after testing.
>>
>> Thanks,
>> Vladimir
>>
>>
>> On 2/19/17 10:52 AM, Lindenmaier, Goetz wrote:
>>> Hi Vladimir,
>>>
>>> I added getShiftCon():
>>> http://cr.openjdk.java.net/~goetz/wr17/8173470-maskShift/webrev.03/
>>>
>>> Best regards,
>>>   Goetz.
>>>
>>>> -----Original Message-----
>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>> Sent: Freitag, 17. Februar 2017 17:59
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-compiler-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR(M): 8173470: [C2] Mask shift operands in ideal graph.
>>>>
>>>> I was think about using find_int_con() and pass in(2) to
>>>> maskedShiftAmount()
>>>>
>>>> static int maskShiftAmount(PhaseGVN *phase, Node *shiftNode, int
>> nBits) {
>>>>    jint       shift = phase->find_int_con(shiftNode, 0); // Use 0 as default for
>>>> the check to work in both cases
>>>>    jint maskedShift = shift & (nBits - 1);
>>>>
>>>>    if (maskedShift == 0) return 0;     // Let Identity() handle 0 shift count.
>>>>
>>>>    if (shift != maskedShift) {
>>>>      shiftNode->set_req(2, phase->intcon(maskedShift)); // Replace shift
>> count
>>>> with masked value.
>>>>    }
>>>>
>>>>    return maskedShift;
>>>> }
>>>>
>>>> And all Identity (with corresponding mask):
>>>>
>>>> Node* LShiftINode::Identity(PhaseGVN* phase) {
>>>>    return (phase->find_int_con(in(2), -1) & (BitsPerJavaInteger - 1)) == 0) ?
>>>> in(1) : this;
>>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 2/16/17 11:25 PM, Lindenmaier, Goetz wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> Thanks for looking at this.
>>>>>
>>>>> I moved the code to a new method "maskShiftAmount"
>>>>> http://cr.openjdk.java.net/~goetz/wr17/8173470-maskShift/webrev.02/
>>>>>
>>>>> Best regards,
>>>>>   Goetz.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
>>>>>> bounces at openjdk.java.net] On Behalf Of Vladimir Kozlov
>>>>>> Sent: Donnerstag, 16. Februar 2017 21:50
>>>>>> To: hotspot-compiler-dev at openjdk.java.net
>>>>>> Subject: Re: RFR(M): 8173470: [C2] Mask shift operands in ideal graph.
>>>>>>
>>>>>> Thank you, Goetz
>>>>>>
>>>>>> Very nice cleanup!
>>>>>>
>>>>>> On 2/16/17 9:03 AM, Lindenmaier, Goetz wrote:
>>>>>>> Hi Lutz,
>>>>>>>
>>>>>>> thanks for the review.
>>>>>>>
>>>>>>> What about naming them con and maskedCon?
>>>>>>
>>>>>> shift and maskedShift
>>>>>>
>>>>>> And since you are doing cleanup ;) I want to ask you to factor out in
>>>> separate
>>>>>> methods (for long and int) checks which
>>>>>> are duplicated in all this methods. All of them check if in(2) is integer
>>>>>> constant and mask it.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>>
>>>>>>> Best regards,
>>>>>>>   Goetz
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Schmidt, Lutz
>>>>>>>> Sent: Donnerstag, 16. Februar 2017 17:55
>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'hotspot-
>>>>>> compiler-
>>>>>>>> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
>>>>>>>> Subject: Re: RFR(M): 8173470: [C2] Mask shift operands in ideal
>> graph.
>>>>>>>>
>>>>>>>> Goetz,
>>>>>>>>
>>>>>>>> Your changes look good. Thanks for cleaning up!
>>>>>>>>
>>>>>>>> I do have one request, though:
>>>>>>>> In the Ideal methods, you are using either local variable names
>>>>>> con/in2_con
>>>>>>>> or shift/in2_shift. Could you please decide for one variant? Thank
>> you.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Lutz
>>>>>>>>
>>>>>>>> From: hotspot-compiler-dev <hotspot-compiler-dev-
>>>>>>>> bounces at openjdk.java.net <mailto:hotspot-compiler-dev-
>>>>>>>> bounces at openjdk.java.net> > on behalf of Götz Lindenmaier
>>>>>>>> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com>
>>>
>>>>>>>> Date: Donnerstag, 16. Februar 2017 um 09:26
>>>>>>>> To: "'hotspot-compiler-dev at openjdk.java.net <mailto:'hotspot-
>>>> compiler-
>>>>>>>> dev at openjdk.java.net> '" <hotspot-compiler-dev at openjdk.java.net
>>>>>>>> <mailto:hotspot-compiler-dev at openjdk.java.net> >
>>>>>>>> Subject: RFR(M): 8173470: [C2] Mask shift operands in ideal graph.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Constant shift operand > 31 (int) or > 63 (long) are pointless.
>>>>>>>>
>>>>>>>> Mask them in the ideal graph, i.e.
>>>>>>>>
>>>>>>>> if a constant is too large replace it by a smaller one.
>>>>>>>>
>>>>>>>> The code also should use BitsPerJavaInteger instead of BitsPerInt.
>>>>>>>>
>>>>>>>> Please review this change. I please need a sponsor.
>>>>>>>> http://cr.openjdk.java.net/~goetz/wr17/8173470-
>> maskShift/webrev.01
>>>>>>>>
>>>>>>>>
>>>>>>>> From the standard:
>>>>>>>> https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-
>> 15.19
>>>>>>>> <https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-
>>>> 15.19>
>>>>>>>> If the promoted type of the left-hand operand is int, only the five
>>>> lowest-
>>>>>>>> order bits of the right-hand operand are used as the shift distance. It
>> is
>>>> as
>>>>>> if
>>>>>>>> the right-hand operand were subjected to a bitwise logical AND
>>>> operator
>>>>>> &
>>>>>>>> (§15.22.1) with the mask value 0x1f (0b11111). The shift distance
>>>> actually
>>>>>>>> used is therefore always in the range 0 to 31, inclusive.
>>>>>>>>
>>>>>>>> If the promoted type of the left-hand operand is long, then only the
>> six
>>>>>>>> lowest-order bits of the right-hand operand are used as the shift
>>>>>> distance. It
>>>>>>>> is as if the right-hand operand were subjected to a bitwise logical
>> AND
>>>>>>>> operator & (§15.22.1) with the mask value 0x3f (0b111111). The shift
>>>>>>>> distance actually used is therefore always in the range 0 to 63,
>>>> inclusive.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>>   Goetz.
>>>>>>>