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

[Lindenmaier, Goetz]

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