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

[Lindenmaier, Goetz]

Hi Vladimir, 

thanks for sponsoring!

Best regards,
  Goetz.

> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Mittwoch, 22. Februar 2017 21:10
> 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.
> 
> 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.
> >>>>>>>