[12] RFR(S) 8215317: [GRAAL] unit test CheckGraalIntrinsics failed after 8213754

[Vladimir Kozlov]

On 1/17/19 6:07 AM, Gustavo Romero wrote:
> Hi Vladimir,
> 
> On 01/16/2019 11:40 PM, Vladimir Kozlov wrote:
>> You should combine both changes and request them at the same time. Changeset will have to list 
>> both changes. Originally your changes should include CheckGraalIntrinsics.java fix.
> 
> Thanks a lot for advising. Just one question: by "Changeset will have to
> list both changes" you mean the commit title (or body) must say explicitly
> the change is a combination of 8213754 + 821531?

Separate lines per bug subject. Here is example:

http://hg.openjdk.java.net/jdk/jdk/rev/c139884bd80e

8213348: jdk.internal.vm.compiler.management service providers missing in module descriptor
8211781: re-building fails after changing Graal sources
Reviewed-by: erikj, mchung

Vladimir

> 
> 
>> Note, for 8213754 corresponding Graal test fix is 8215317 (and not 8215687, that one is for 
>> 8212043 Math.min/max).
> 
> Thanks for the detailed note. I commented on the right thread but pasted
> the wrong bug!
> 
> Thank you and best regards,
> Gustavo
> 
>> Yes, Graal fix in jdk11u is different - new intrinsics should be listed under existing condition 
>> isJDK11OrHigher():
>>
>> http://hg.openjdk.java.net/jdk-updates/jdk11u/file/5fc74655f16d/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java#l371 
>>
>>
>> Regards,
>> Vladimir
>>
>> On 1/16/19 1:57 PM, Gustavo Romero wrote:
>>> Hi Vladimir,
>>>
>>> I would like to request the approval to backport the change:
>>>
>>> 8213754: PPC64: Add Intrinsics for isDigit/isLowerCase/isUpperCase/isWhitespace
>>> https://bugs.openjdk.java.net/browse/JDK-8213754
>>>
>>> to jdk11u, but if it gets integrated before 8215687 it will break Graal
>>> test HotspotTest.java/CheckGraalIntrinsics.java again, as expected.
>>>
>>> Are you fine if I request the approval to backport first this change, i.e.
>>> 8215687?
>>>
>>> Actually I'll have to tweak a bit and s/isJDK12OrHigher/isJDK11OrHigher/,
>>> right?
>>>
>>> Thank you.
>>>
>>> Best regards,
>>> Gustavo
>>>
>>> On 12/13/2018 02:10 AM, Vladimir Kozlov wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8215317
>>>>
>>>> JDK-8213754 added new intrinsics which cause Graal's unit test failure.
>>>>
>>>> CheckGraalIntrinsics test is adjusted for new intrinsics:
>>>>
>>>> src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/CheckGraalIntrinsics.java 
>>>>
>>>> @@ -376,6 +376,14 @@
>>>>                               "jdk/jfr/internal/JVM.getEventWriter()Ljava/lang/Object;");
>>>>           }
>>>>
>>>> +        if (isJDK12OrHigher()) {
>>>> +            add(toBeInvestigated,
>>>> +                            "java/lang/CharacterDataLatin1.isDigit(I)Z",
>>>> +                            "java/lang/CharacterDataLatin1.isLowerCase(I)Z",
>>>> +                            "java/lang/CharacterDataLatin1.isUpperCase(I)Z",
>>>> +                            "java/lang/CharacterDataLatin1.isWhitespace(I)Z");
>>>> +        }
>>>> +
>>>>           if (!config.inlineNotify()) {
>>>>               add(ignore, "java/lang/Object.notify()V");
>>>>           }
>>>>
>>>> Tested tier1 and tier3-graal (where test is run).
>>>>
>>>> I also pushed changes into Lab's Graal repo so this test will be updated during next sync.
>>>> But I want to push fix into JDK because JDK 12 will be forked very soon.
>>>>
>>>
>>
>