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.ver...@oracle.com>
À: "Claes Redestad" <claes.redes...@oracle.com>, "core-libs-dev"
<core-libs-dev@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
