RFR 8233920: MethodHandles::tryFinally generates illegal bytecode for long/double return types

[Jorn Vernee]

Hi Rémi, Claes,

Thanks for the suggestion, Rémi. I avoided using a store/load for all 
types since I didn't want to alter the existing code shape. But, using 
store/load for all types simplifies the generator code & javadoc, so it 
would be nice to do that I think. However, this might have other effects 
on peak/link-time performance as well.

Given that using store/load aligns the code-shape with what javac does, 
I think chances are good that the JIT will have an easier time 
optimizing that code shape. That said, it really needs more investigation.

Here is an updated webrev that uses store/load for all return types: 
http://cr.openjdk.java.net/~jvernee/8233920/webrev.03/

As long as Claes is okay with it, I think we should go for that one 
since the code/doc is simpler. I can file an RFE for examining the 
performance of different code shapes.

Thanks,
Jorn

On 12/11/2019 16:35, Claes Redestad wrote:
> Interesting, and you might be right. However I think it'd be better to
> file a follow-up to examine this (pre-existing) behavior than to 
> expand the scope of this bug fix.
>
> /Claes
>
> On 2019-11-12 16:08, Remi Forax wrote:
>> Hi Jorn, Claes,
>> Is it not better to always store in a local variable, like javac 
>> does, instead of doing the double SWAP if there is a return value of 
>> size 1 ?
>>
>> Rémi
>>
>> ----- Mail original -----
>>> De: "Jorn Vernee" <jorn.vernee at oracle.com>
>>> À: "Claes Redestad" <claes.redestad at oracle.com>, "core-libs-dev" 
>>> <core-libs-dev at openjdk.java.net>
>>> Envoyé: Mardi 12 Novembre 2019 15:56:06
>>> Objet: Re: RFR 8233920: MethodHandles::tryFinally generates illegal 
>>> bytecode for long/double return types
>>
>>> Hi Claes,
>>>
>>> Thanks for the comments!
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.02/
>>>
>>> Also, thanks for sponsoring.
>>>
>>> Jorn
>>>
>>> On 12/11/2019 15:30, Claes Redestad wrote:
>>>> Hi Jorn,
>>>>
>>>> good catch!
>>>>
>>>> Some minor stylistic concerns:
>>>>
>>>> "* = depends on whether the return type is a category 2 type, which
>>>> takes up 2 stack slots."
>>>>
>>>> While long/double are known to use two stack slots, "category 2 type"
>>>> isn't used much outside the specification. I'd either simplify to
>>>> "* = depends on whether the return type takes up 1 or 2 stack slots."
>>>> or add a reference to the definition of type categories.
>>>>
>>>> Similarly I think:
>>>>
>>>> boolean returnsCat2Type = returnType == long.class || returnType ==
>>>> double.class;
>>>>
>>>> could be simplified as:
>>>>
>>>> boolean isDoubleWord = basicReturnType.isDoubleWord(); //
>>>> returnsDoubleWord?
>>>>
>>>> In the test, is it possible to narrow the expected exception to the
>>>> exact type being thrown (T1?) rather than Throwable?
>>>>
>>>> I can sponsor the patch.
>>>>
>>>> /Claes
>>>>
>>>> On 2019-11-12 12:09, Jorn Vernee wrote:
>>>>> Hi,
>>>>>
>>>>> Please review the following patch that fixes handling of long/double
>>>>> returns for the tryFinally MethodHandle combinator.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8233920
>>>>> Webrev: http://cr.openjdk.java.net/~jvernee/8233920/webrev.01/
>>>>> (Testing=tier1)
>>>>>
>>>>> FWIW, I also added some missing close tags to the javadoc in the
>>>>> InvokerBytecodeGenerator class (IntelliJ was warning about this).
>>>>>
>>>>> As a heads-up; I'm not a committer on the jdk project, so a sponsor
>>>>> would be needed to push this.
>>>>>
>>>>> Thanks,
>>>>> Jorn