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

[Lindenmaier, Goetz]

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