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

[Volker Simonis]

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