hotspot: problem with bitwise rotation on PowerLE

Wed May 13 09:36:40 UTC 2015

Thanks, but I've already written a minimal test case myself.

I just asked for including the initial test by Jan because it also
exercises some other interesting parts of the optimizer and may thus
be a valuable test case in itself.

Regards,
Volker

On Wed, May 13, 2015 at 12:34 AM, Alexander Smundak <asmundak at google.com> wrote:
> I have somewhat more isolated test (just calling
> rotateRigth/rotateLeft methods from my previous e-mail with constant
> distance argument that reveals the problem. Please let me know if you
> are interested.
>
> On Tue, May 12, 2015 at 2:17 PM, Jan S Rellermeyer
> <rellermeyer at us.ibm.com> wrote:
>> I have also been able test the patch in our setup and it works. Thanks for
>> figuring this out.
>>
>> Do you happen to know when the next 8 update release is scheduled that would
>> contain the fix?
>>
>> Regarding coverage by the IBM OCA and the ability to contribute the hash
>> code for regression testing, I am checking this right now with the people
>> who passed the code on to me.
>>
>> --Jan.
>>
>> Alexander Smundak ---05/12/2015 03:59:48 PM---I can confirm that the patch
>> works. I tried to write the test that covers both 'rotlI_reg_immi8' and
>>
>> From: Alexander Smundak <asmundak at google.com>
>> To: Volker Simonis <volker.simonis at gmail.com>
>> Cc: Jan S Rellermeyer/Austin/IBM at IBMUS, "ppc-aix-port-dev at openjdk.java.net"
>> <ppc-aix-port-dev at openjdk.java.net>
>> Date: 05/12/2015 03:59 PM
>> Subject: Re: hotspot: problem with bitwise rotation on PowerLE
>>
>> ________________________________
>>
>>
>>
>> I can confirm that the patch works. I tried to write the test that covers
>> both
>> 'rotlI_reg_immi8' and ' rotrI_reg_immi8' patterns but I didn't manage
>> to match the latter,
>> somehow all of the following methods inline to ROTLWI:
>>
>>  static int rotateRight1(int i, int distance) { return (i >>>
>> distance) | (i << -distance); }
>>  static int rotateRight2(int i, int distance) { return (i <<
>> -distance) | (i >>> distance); }
>>
>> These two inline to
>>    rotlwi Rx, Ry, (32-distance)
>>
>>  static int rotateLeft1(int i, int distance) { return (i << distance)
>> | (i >>> -distance); }
>>  static int rotateLeft2(int i, int distance) { return (i >>>
>> -distance) | (i << distance); }
>> And these to inline to
>>    rotlwi Rx, Ry, distance
>>
>> How can such a bug escape the hotspot test? I don't have access to
>> JCK, but isn't it supposed to cover this?
>>
>> On Tue, May 12, 2015 at 11:31 AM, Volker Simonis
>> <volker.simonis at gmail.com> wrote:
>>> Hi,
>>>
>>> I think I've finally understood and fixed this issue.
>>>
>>> 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. That's why
>>> you saw the assertion in the fastdebug build.
>>>
>>> I think the right solution for this problem is to simply mask the
>>> rotation distance with 0x1f  (which is equivalent to taking the
>>> distance modulo 32).
>>>
>>> Can you please verify that the proposed fix also works for you on LE?
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8080190
>>> http://cr.openjdk.java.net/~simonis/webrevs/2015/8080190/
>>>
>>> If yes I can send it out for review and push it to our jdk9 and jdk8u
>>> repositories.
>>>
>>> @Jan: have you signed the Oracle Contributor Agreement or are you
>>> covered by the company-wide IBM OCA? I'd like to take your test as a
>>> regresstion test into the OpenJDK HotSpot repository, but this is only
>>> possible if you're a Contributor[2].
>>>
>>> Thank you and best regards,
>>> Volker
>>>
>>> PS: the issue didn't appear in our internal SAP JVM version because we
>>> already mask the inputs of the corresponding shift nodes in the ideal
>>> graph but for some unknown reasons this shared changes haven’t been
>>> contributed along with our ppc port so far.
>>>
>>> [1] http://www.oracle.com/technetwork/community/oca-486395.html
>>> [2] http://openjdk.java.net/bylaws#contributor
>>>
>>>
>>> On Tue, May 12, 2015 at 2:38 PM, Volker Simonis
>>> <volker.simonis at gmail.com> wrote:
>>>> Hi,
>>>>
>>>> this problem is not LE-related. I could reproduce it on BE as well. I
>>>> also don't think that the fix proposed by Jan is completely right. I
>>>> think it only helps because it leads to another instruction being
>>>> matched. The problem is that the URshift has a negative input which
>>>> should not be. It should have been converted to it's positive
>>>> equivalent modulo 32 (as described in the  JLS 15.19 -
>>>> https://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.19).
>>>>
>>>> Strange enough, in our internal version, the error doesn't occur so I
>>>> should actually easy find the difference. Unfortunately, I didn't saw
>>>> it until now :(
>>>> I'll keep you posted once I found out more...
>>>>
>>>> Regards,
>>>> Volker
>>>>
>>>> On Tue, May 12, 2015 at 6:06 AM, Alexander Smundak <asmundak at google.com>
>>>> wrote:
>>>>> Looking into this. Sorry for the late response, we were moved into a
>>>>> different office over the weekend.
>>>>>
>>>>> On Fri, May 8, 2015 at 1:01 PM, Jan S Rellermeyer
>>>>> <rellermeyer at us.ibm.com> wrote:
>>>>>> I am helping people in IBM who have encountered a hotspot bug that
>>>>>> surfaces
>>>>>> when using bitwise rotations (such as through Integer.rotateRight). I
>>>>>> have
>>>>>> attached a small test program that illustrates the problem. When
>>>>>> running
>>>>>> this on Power LE (I don't think that anybody tried on BE, though) the
>>>>>> results of the hash function are incorrect.
>>>>>>
>>>>>> The bug goes away when disabling inlining. After digging through the
>>>>>> generated code I found out that it contained some suspicious fnmadd
>>>>>> instructions where you would rather expect some kind of left rotate. I
>>>>>> could
>>>>>> track down that the bug was caused by the immediate value for the right
>>>>>> shift constant of what should be a rlwinm node to be negative and
>>>>>> thereby
>>>>>> bleeding into the opcode (which unfortunately still created a valid
>>>>>> Power
>>>>>> instruction, namely the fnmadd). Running with a fastdebug build causes
>>>>>> an
>>>>>> assertion to fail.
>>>>>>
>>>>>> The attached patch does fix the problem and so does an alternative
>>>>>> patch
>>>>>> that replaces the URshift with a UShift but I am not entirely sure if
>>>>>> this
>>>>>> really fixes the root cause or rather just prevents the rotlI
>>>>>> instruction
>>>>>> from being matched by the ReduceInst.
>>>>>>
>>>>>> --Jan.
>>>>>>
>>>>>>
>>>>>> Dr. Jan S. Rellermeyer
>>>>>> IBM Research
>>>>>> RSM - Austin Research Lab
>>>>>> rellermeyer_at_us.ibm.com
>>>>>> The University of Texas at Austin
>>>>>> Adjunct Assistant Professor
>>>>>> jrellerm_at_cs.utexas.edu
>>>>>>
>>>>>> (See attached file: hash.java)(See attached file: rotl.patch)
>>
>>
>>