RFR (12) JDK-8218025: disable pop_frame and force_early_return caps for Graal

[Igor Ignatyev]

Hi Alex,

sure I understand that, but let's say we decide to add these capabilities to ZERO or remove all ZERO-specific code. I am *not* saying we are going to do that, just speculating. In such cases, your code[1] will require more effort to figure that should be changed comparing to the almost identical code[2]. but as I said I'm fine w/ your current code.

Thanks,
-- Igor


[1]
>  #ifndef ZERO
>    jc.can_pop_frame = 1;
>    jc.can_force_early_return = 1;
> +  // Workaround for 8195635:
> +  // disable pop_frame and force_early_return capabilities with Graal
> +#if INCLUDE_JVMCI
> +  if (UseJVMCICompiler) {
> +    jc.can_pop_frame = 0;
> +    jc.can_force_early_return = 0;
> +  }
> +#endif // INCLUDE_JVMCI
>  #endif // !ZERO

[2]
>  #ifndef ZERO
>    jc.can_pop_frame = 1;
>    jc.can_force_early_return = 1;
>  #endif // !ZERO
> +  // Workaround for 8195635:
> +  // disable pop_frame and force_early_return capabilities with Graal
> +#if INCLUDE_JVMCI
> +  if (UseJVMCICompiler) {
> +    jc.can_pop_frame = 0;
> +    jc.can_force_early_return = 0;
> +  }
> +#endif // INCLUDE_JVMCI



> On Jan 31, 2019, at 10:59 AM, Alex Menkov <alexey.menkov at oracle.com> wrote:
> 
> 
> 
> On 01/31/2019 10:52, Igor Ignatyev wrote:
>> Hi Alex,
>> you have 'if INCLUDE_JVMCI' inside 'ifndef ZERO', although we currently don't have zero builds w/ JVMCI (and I don't think such builds would make much sense), it's better not to rely on that.  not a blocker from my point of view though.
> 
> can_pop_frame & can_force_early_return are set only if ZERO is not defined (for zero builds the capabilities are not supported), so I added disabling logic in the same block.
> 
> --alex
> 
>> Thanks,
>> -- Igor
>>> On Jan 31, 2019, at 10:38 AM, Alex Menkov <alexey.menkov at oracle.com> wrote:
>>> 
>>> Hi guys,
>>> 
>>> thank you for the feedback.
>>> 
>>> updated webrev (used the way suggested by David & Igor):
>>> http://cr.openjdk.java.net/~amenkov/tck_red_disable_caps/webrev.02/
>>> 
>>> --alex
>>> 
>>> On 01/30/2019 21:31, serguei.spitsyn at oracle.com wrote:
>>>> On 1/30/19 21:24, David Holmes wrote:
>>>>> On 31/01/2019 3:18 pm, dean.long at oracle.com wrote:
>>>>>> On 1/30/19 8:59 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> So, the fix needs to be more like this:
>>>>>>> + // Workaround for 8195635:
>>>>>>> + // disable pop_frame and force_early_return capabilities with Graal
>>>>>>> + #if INCLUDE_JVMCI
>>>>>>> + if (!(EnableJVMCI && UseJVMCICompiler)) {
>>>>>>>     jc.can_pop_frame = 1;
>>>>>>>     jc.can_force_early_return = 1;
>>>>>>> + } + #endif Not sure, if the check for EnableJVMCI can be removed above.
>>>>>> 
>>>>>> We still need it to work when INCLUDE_JVMCI is not defined.
>>>>>> How about
>>>>>> 
>>>>>> JVMCI_ONLY(if (UseJVMCICompiler)) {
>>>>>> ...
>>>>>> }
>>>>>> 
>>>>>> or
>>>>>> 
>>>>>> if (JVMCI_ONLY(UseJVMCICompiler) NOT_JVMCI(true)) {
>>>>>> ...
>>>>>> }
>>>>> 
>>>>> Or just turn them on unconditionally first and turn off explicitly for JVMCI:
>>>>> 
>>>>>  jc.can_pop_frame = 1;
>>>>>  jc.can_force_early_return = 1;
>>>>> + #if INCLUDE_JVMCI
>>>>> +  // Workaround for 8195635:
>>>>> +  // disable pop_frame and force_early_return capabilities with Graal
>>>>> + if (EnableJVMCI && UseJVMCICompiler) {
>>>>> +     jc.can_pop_frame = 0;
>>>>> +     jc.can_force_early_return = 0;
>>>>> + }
>>>>> + #endif
>>>>> 
>>>> Oh, Dean is right.
>>>> We need these caps initialized even if the macro INCLUDE_JVMCI is undefined.
>>>> Then I like variant from David above.
>>>> Thanks,
>>>> Serguei
>>>>> David
>>>>> 
>>>>>> dl
>>>>>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190131/c15dd24d/attachment.html>