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

[Vladimir Kozlov]

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.
>>>>>