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

[Vladimir Kozlov]

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