RFR: 8257020: [JVMCI] enable a JVMCICompiler to specify which GCs it supports [v2]

[Vladimir Kozlov]

On Wed, 25 Nov 2020 14:53:34 GMT, Doug Simon <dnsimon at openjdk.org> wrote:

>>> > Could this be done without adding JVMCI code to GCConfig::is_gc_supported?
>>> 
>>> This has to go through the WhiteBox API as JVMCI is not accessible to normal Java code (such as VMProps).
>>> We could restrict the JVMCI interposition to WhiteBox. This is what I did [initially](https://github.com/openjdk/jdk/pull/1423/commits/191d61d996dd08f43c2c946f02b0cce0d96f2ef4). However, I then saw that `GCConfig::is_gc_supported` is only called from whitebox.cpp so [moved](https://github.com/openjdk/jdk/pull/1423/commits/54b7ba8463dfee9a679abb09a5cc898ba8550d85) the logic directly into `GCConfig`. I think this is better because as soon as there's another caller of `GCConfig::is_gc_supported`, they should get an accurate answer and not have to worry about implications of `EnableJVMCI` separately.
>> 
>> We already have code in place to ensure that EnableJVMCI is set to false when you run a GC that doesn't support JVMCI:
>> void JVMCIGlobals::check_jvmci_supported_gc() {
>>   if (EnableJVMCI) {
>>     // Check if selected GC is supported by JVMCI and Java compiler
>>     if (!(UseSerialGC || UseParallelGC || UseG1GC)) {
>>       vm_exit_during_initialization("JVMCI Compiler does not support selected GC", GCConfig::hs_err_name());
>>       FLAG_SET_DEFAULT(EnableJVMCI, false);
>>       FLAG_SET_DEFAULT(UseJVMCICompiler, false);
>>     }
>>   }
>> }
>> 
>> So, in our view we have "if you run this particular GC, that don't have support in JVMCI, then we turn off JVMCI forcefully", and we rely on that behavior in the HotSpot code. The proposed patch turns this inside out and the changed is_gc_supported function now becomes "if you've turned on EnableJVMCI is this GC supported". It sort-of inverts the dependency between the two sub-systems and we don't want to use that particular question in HotSpot. AFAICT, this is something you only need to ask in the context of jtreg's @requires, so I think it should be moved out of the GC code and put in the jtreg extension code.
>> 
>> We've other flags that we treat similarly. For example, UseCompressedOops. ZGC doesn't support it, so we turn it off forcefully, but we don't have code in GCConfig::is_gc_supported that returns false when UseCompressedOops is turned on. That is completely handled by the jtreg extensions (and the WhiteboxAPI).
>> 
>> I think you could easily solve this without adding calls through JVMCI from the GC code by:
>> 1) Update JVMCIGlobals::supported_gc() to make a call through JVMCI into the used compiler that can tell if it supports the particular GC
>> 2) Use Vladimirs proposed check_jvmci_supported_gc function, which uses supported_gc().
>> 3) Add your own  WhiteboxAPI function, say WB_IsJVMCISupportedByGC, and call your "supported_gc" function from that
>> 4) Update VMProps.java like I suggested above
>
> `JVMCIGlobals::check_jvmci_supported_gc` is incorrect in that it hard codes the GCs supported by JVMCI which not something JVMCI itself can accurately answer. What's more, the upcall into JVMCI Java code to get the right answer cannot be made here as it is too early in the VM boot process. There's not even a `JavaThread` available yet. This code should be removed and let Graal exit the VM with an error should it be used in conjunction with a GC it does not support. For example:
> Error occurred during initialization of VM
> JVMCI Compiler does not support selected GC: epsilon gc
> 
> Moving the check to later in the VM bootstrap doesn't really work either as it imposes the overhead of initializing JVMCI and Graal eagerly.
> This means a conflict between GC and JVMCI compiler cannot be finessed by overriding the compiler selection to use a non-JVMCI compiler. It's a fatal VM error, just like selecting multiple GCs on the command line is.
> 
> If you agree with that, then I think the proposed PR is the right approach. We could revert to the version that leaves the JVMCI specific logic in `WB_IsGCSupported` but as I stated, that means `GCConfig::is_gc_supported` can give a wrong answer.

I think you merged 2 things into this PR which made me confused. The title says that Graal should specify which GC it supports (new JVMCI API). But your description is about fixing Graal testing with different GCs. Yes, they are connected but I think they should be reviewed separately.

The reason currently we test Graal supported GC in C++ code during VM startup is because we want to avoid executing application in Interpreter and bailout later when Graal is loaded to compile a hot method. Note, Graal in JDK is loaded on first compilation request.
To get answer from Graal about GC you would have to load and initialize it very early which will affect startup (even with libgraal).

To add new JVMCI API just to resolve testing issues is overkill for me.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1423