hotspot: problem with bitwise rotation on PowerLE

Wed May 20 13:50:07 UTC 2015

I have now also fixed this issue in our ppc-aix-port/jdk7u repository

http://hg.openjdk.java.net/ppc-aix-port/jdk7u/hotspot/rev/c1394d3bdb6a

Please consider pulling this change into your local repositories if
you are building on Linux/ppc64 or AIX because it will not be
integrated into the main jdk7u/hotspot repository (simply because the
main jdk7u/hotspot does not yet contain the ppc64 port).

Regards,
Volker

On Wed, May 20, 2015 at 12:37 PM, Tony Reix <tony.reix at atos.net> wrote:
> What about jdk7 ?
>
>
> Cordialement,
>
> Tony Reix
>
> Bull - ATOS
> IBM Coop Architect & Technical Leader
> Office : +33 (0) 4 76 29 72 67
> 1 rue de Provence - 38432 Échirolles - France
> www.atos.net
>
> ________________________________________
> De : ppc-aix-port-dev [ppc-aix-port-dev-bounces at openjdk.java.net] de la part de Volker Simonis [volker.simonis at gmail.com]
> Envoyé : mercredi 20 mai 2015 10:45
> À : Alexander Smundak
> Cc : ppc-aix-port-dev at openjdk.java.net
> Objet : Re: hotspot: problem with bitwise rotation on PowerLE
>
> I just wanted to let you know that this issue has now been fixed in
> jdk9 and jdk8u.
>
> Currently the change for 9 is in the jdk9/hs-comp/hotspot team
> repository but it should propagate to jdk9/dev/hotspot soon (in a week
> or two).
>
> The fix for 8u is currently in jdk8u/hs-dev/hotspot but should
> propagate to jdk8u/dev in time for the upcoming 8u60 release.
>
> You can watch the status of the fix at:
>
> https://bugs.openjdk.java.net/browse/JDK-8080190 for jdk9 and:
> https://bugs.openjdk.java.net/browse/JDK-8080749 for the 8u downport.
>
> Thanks for reporting this.
> Volker
>
>
> On Wed, May 13, 2015 at 11:36 AM, Volker Simonis
> <volker.simonis at gmail.com> wrote:
>> 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)
>>>>
>>>>
>>>>