RFR(S): 8080190: PPC64: Fix wrong rotate instructions in the .ad file

[Vladimir Kozlov]

You need to change the comment in ppc.ad.
Otherwise looks good. Thank you for doing archeological research - I 
forgot about that change.

Thanks,
Vladimir

On 5/18/15 9:05 AM, Volker Simonis wrote:
> Hi Vladimir,
>
> thanks for the review and sorry for the late reply (we had a public
> holiday in Germany:).
>
>
> On Wed, May 13, 2015 at 8:39 PM, Vladimir Kozlov
> <vladimir.kozlov at oracle.com> wrote:
>> Is this because you can have negative values?
>> I see that at the bottom of calls which generate bits u_field() method is
>> used. Why not mask it there?
>>
>
> u_field() generates immediates in the range (hi_bit - lo_bit + 1) but
> it can be used to generate not only 5-bit immediates but also bigger
> or smaller ones (e.g. tbr() calls u_field() to generate a 10-bit
> immediate. The assertion in u_field() checks that the given value can
> be encoded in the requested range of bits but we do not want to
> explicitly mask the value to the corresponding bit range as this could
> hide other problems.
>
> The other problem is that we can have negative values as you have
> correctly noticed.
>
> I think this is more a kind of a conceptual problem. The JLS states
> that "only the five lowest-order bits of the right-hand operand are
> used as the shift distance" in a shift operation [1]. The same holds
> true for the specifications of the shift bytecodes in the JVMS [2].
> But for constant shift amounts we do not handle this in C2 (i.e. in
> the ideal graph) but instead we relay on the corresponding CPU
> instruction doing "the right thing". On x86 this works because the
> whole 8-bit immediates are encoded right into the instruction and
> interpreted as required by the JLS.
>
> Another way to solve this would be to do the masking right in the
> ideal graph as we've previously done in the SAP JVM:
>
> Node *LShiftINode::Ideal(PhaseGVN *phase, bool can_reshape) {
>    const Type *t  = phase->type( in(2) );
>    if( t == Type::TOP ) return NULL;       // Right input is dead
>    const TypeInt *t2 = t->isa_int();
>    if( !t2 || !t2->is_con() ) return NULL; // Right input is a constant
>
>    // SAPJVM: mask constant shift count already in ideal graph processing
>    const int in2_con = t2->get_con();
>    const int con     = in2_con & ( BitsPerJavaInteger - 1 );   //
> masked shift count
>
>    if ( con == 0 )  return NULL; // let Identity() handle 0 shift count
>
>    if( in2_con != con ) {
>      set_req( 2, phase->intcon(con) ); // replace shift count with masked value
>    }
>
> Damned! I just wanted to ask you what's your opinion about this
> solution but while writing all this and doing some software
> archaeology I just discovered that this problem has already been
> discussed [3] and solved some time ago! Change "7029017: Additional
> architecture support for c2 compiler" introduced  the "Support (for)
> masking of shift counts when the processor architecture mandates it".
> This is exactly what's needed on PowerPC!
>
> So I'd like to change my fix as follows:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2015/8080190.v3/
>
> I think that using "Matcher::need_masked_shift_count = true" will make
> a lot of the masking for shift instructions in the ppc.ad file
> obsolete, but I'd like to keep these cleanups for a separate change.
>
> Regards,
> Volker
>
> [1] https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.19
> [2] https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.ishl
> [3] http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2011-March/thread.html#4944
>
>> There are other instructions which can have the same problem (slwi). You
>> would have to guarantee all of those do the masking. That is why I am asking
>> why not mask it at the final code methods, like u_field()?
>>
>> Thanks,
>> Vladimir
>>
>>
>> On 5/13/15 9:29 AM, Volker Simonis wrote:
>>>
>>> Hi,
>>>
>>> could you please review the following, ppc64-only fix which solves a
>>> problem with the integer-rotate instructions in C2:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8080190
>>> http://cr.openjdk.java.net/~simonis/webrevs/2015/8080190.v2/
>>>
>>> The problem was that we initially took the "rotate left/right by 8-bit
>>> immediate" instructions from x86_64. But on x86_64 the rotate
>>> instructions for 32-bit integers do in fact really take 8-bit
>>> immediates (seems like the Intel guys had to many spare bits when they
>>> designed their instruction set :) On the other hand, the rotate
>>> instructions on Power only use 5 bit to encode the rotation distance.
>>>
>>> We do check for this limit, but only in the debug build so you'll see
>>> an assertion there, but in the product build we will silently emit a
>>> wrong instruction which can lead to anything starting from a crash up
>>> to an incorrect computation.
>>>
>>> I think the simplest solution for this problem is to mask the
>>> rotation distance with 0x1f  (which is equivalent to taking the
>>> distance modulo 32) in the corresponding instructions.
>>>
>>> This change should also be backported to 8 as fast as possible. Notice
>>> that this a day-zero bug in our ppc64 port because in our internal SAP
>>> JVM we already mask the shift amounts in the ideal graph but for some
>>> unknown reasons these shared changes haven’t been contributed along
>>> with our port.
>>>
>>> Thank you and best regards,
>>> Volker
>>>
>>