hotspot: problem with bitwise rotation on PowerLE

Tue May 12 22:34:14 UTC 2015

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