[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR(S): 8080190: PPC64: Fix wrong rotate instructions in the .ad file
From:       Volker Simonis <volker.simonis () gmail ! com>
Date:       2015-05-18 16:53:14
Message-ID: CA+3eh10pG0ObNgt=4hYKwL_-=hCRXF4Lxvr=s+RVXhzuyzJOYA () mail ! gmail ! com
[Download RAW message or body]

Thanks for the review!
I'll update the comment accordingly.

Regards,
Volker


On Mon, May 18, 2015 at 6:42 PM, Vladimir Kozlov
<vladimir.kozlov@oracle.com> wrote:
> 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@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
>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]