RFR/RFC 8231503: [TESTBUG] compiler/{jvmci, aot} tests should not run with GCs that do not support JVMCI/AOT

[Stefan Karlsson]

Hi Vladimir,

Thanks for the clarification!

I do find this odd and sense a kind of false abstraction here, but I'm 
not going to push for a change in this area. I'm happy that it's going 
to be easier to run ZGC tests locally with this change.

Thanks,
StefanK

On 2019-09-30 19:51, Vladimir Kozlov wrote:
> -XX:-UseJVMCICompiler is used explicitly to run Graal's unit tests to 
> compare results with C2. Without it Graal will be loaded and used 
> instead of C2 by default if +UseJVMCI is specified. These unit tests 
> should not run with not support GCs.
> 
> I don't think we should modify check in 
> JVMCIGlobals::check_jvmci_supported_gc().
> 
> I am fine with proposed changes in VMProps.java and I agree that JVMCI 
> and AOT may have different GCs support even so currently they match.
> 
> Thanks,
> Vladimir
> 
> On 9/30/19 6:27 AM, Stefan Karlsson wrote:
>> [I wrote this before realizing that the mail thread had forked. Some 
>> of the comments might have already been covered]
>>
>> Hi Aleksey,
>>
>> Thanks for fixing this!
>>
>> Inlined:
>>
>> On 2019-09-27 17:26, Aleksey Shipilev wrote:
>>> Replying with proper "To:". Friday, eh.
>>>
>>> On 9/27/19 5:23 PM, Aleksey Shipilev wrote:
>>>> Testbug:
>>>>    https://bugs.openjdk.java.net/browse/JDK-8231503
>>>>
>>>> Candidate fix:
>>>>    https://cr.openjdk.java.net/~shade/8231503/webrev.01/
>>
>> One suggestion would be to rewrite the GC checking to use the enum 
>> values instead of string compares:
>>
>>          switch (GC.selected()) {
>>              case Serial:
>>              case Parallel:
>>              case G1:
>>                  // These GCs are supported with AOT
>>                  return "true";
>>              default:
>>                  // Every other GC is not supported
>>                  return "false";
>>          }
>>
>>
>> Maybe even create a function that returns a boolean.
>>
>>>>
>>>> I am actually not sure it should be fixed this way. The alternative 
>>>> is to add the @requires lines to
>>>> every affected test (there are about a hundred of them), like this:
>>>>    @requires (vm.gc == "null" || vm.gc == "G1" || vm.gc = "Serial" 
>>>> || vm.gc = "Parallel")
>>>>
>>>> ...but that seems too intrusive. Or maybe not? Open for opinions 
>>>> from those who maintain JVMCI/AOT.
>>
>> I would also be interested in hearing what the JVMCI/AOT maintainers 
>> think.
>>
>> Isn't it odd that we prevent these tests from being run when JVMCI is 
>> enabled? Shouldn't we only skip the tests when an unsupported compiler 
>> is used? For all practical purposes that would be when Graal is used.
>>
>> A lot of the tests are run with -XX:-UseJVMCICompiler and it's not 
>> obvious to me that those have to be excluded when running with ZGC or 
>> Shenandoah.
>>
>> I think there might be a bug in check_jvmci_supported_gc and that we 
>> probably should change this to:
>> ---
>> diff --git a/src/hotspot/share/jvmci/jvmci_globals.cpp 
>> b/src/hotspot/share/jvmci/jvmci_globals.cpp
>> --- a/src/hotspot/share/jvmci/jvmci_globals.cpp
>> +++ b/src/hotspot/share/jvmci/jvmci_globals.cpp
>> @@ -123,7 +123,7 @@
>>   }
>>
>>   void JVMCIGlobals::check_jvmci_supported_gc() {
>> -  if (EnableJVMCI) {
>> +  if (EnableJVMCI && UseJVMCICompiler) {
>>       // Check if selected GC is supported by JVMCI and Java compiler
>>       if (!(UseSerialGC || UseParallelGC || UseParallelOldGC || 
>> UseG1GC)) {
>>         vm_exit_during_initialization("JVMCI Compiler does not support 
>> selected GC", GCConfig::hs_err_name());
>> ---
>>
>> I tested this with ZGC (without your patch) and ran the tests in 
>> test/hotspot/jtreg/compiler/jvmci. 90% of the tests passed, some of 
>> the tests failed (as expected) because they actually ran with 
>> -XX:+UseJVMCICompiler, and some (unexpectedly) crashed with 
>> -XX:-UseJVMCICompiler. The latter might be a real bug in the JVMCI API.
>>
>> With all this said, maybe we should:
>>
>> 0) Add your "gc supports graal" checks to a new helper function 
>> (selectedGCSupportsGraal() ?).
>> 1) Use the UseJVCMICompiler patch above
>> 2) Leave the vmJvmci function as is
>> 3) Tag tests actually requiring graal with vm.graal.enabled
>> 4) Maybe change @requires vm.aot to to use vm.aot.enabled
>> 5) Use this function to implement vm.graal.enabled and vm.aot.enabled
>>
>> Of course, I could have misunderstood the meaning of 
>> -XX:-UseJVMCICompiler, but then it would be good to get some 
>> clarification about that.
>>
>> Also, if you are in a hurry to get this resolved, then we could 
>> consider making these changes after your patch has been pushed.
>>
>> Thanks,
>> StefanK
>>
>>>>
>>>> Testing: compiler/aot, compiler/jvmci {serial, parallel, g1, 
>>>> shenandoah, zgc}; tier1
>>>