Attempt to inline by name matcher methods - #19631
Conversation
| @@ -0,0 +1,11 @@ | |||
| final class ByName1(val x: Int) extends AnyVal: | |||
| inline def isEmpty: Boolean = false | |||
There was a problem hiding this comment.
This only seems to be a problem when isEmpty returns a literal, or calls a function returning a literal type; could it be a widening or an extension method is not being applied properly?
| x match | ||
| case ByName1(s) => s | ||
|
|
||
| final class ByName2(val x: Int) extends AnyVal: |
There was a problem hiding this comment.
This case actually produces valid scala code with -Ycheck:all disabled.
| tree match | ||
| case tree: Select if PatternMatcher.isPatmatGenerated(tree) => | ||
| // Purposefully skip any nodes introduced by the pattern matcher. We | ||
| // will inline them at a later stage. |
There was a problem hiding this comment.
This will not work. Everything must be inlined after this stage.
There was a problem hiding this comment.
Could we inline where the calls are introduced then i.e. during the pattern matcher?
Or do we need to do something like the trick that has been done with the unapply body currently and reduced in the InlinePatterns stage?
There was a problem hiding this comment.
Could we inline where the calls are introduced then i.e. during the pattern matcher?
That can be problematic. The code that is inlined assumes that we have not been transformed by the phases that come after inlining (and before pattern matching).
Or do we need to do something like the trick that has been done with the unapply body currently and reduced in the InlinePatterns stage?
That trick will not work well for name-based pattern matching.
There was a problem hiding this comment.
If you really need to remove the code that you generate in #19577 we might need another way. Maybe a possible approach is to change the pattern matcher to do something specific for methods on records.
There was a problem hiding this comment.
Am I right in thinking that it would require a change to the spec? Would there be appetite to add something Java-specific to the spec? It feels quite unsatisfying to need to have a Java-specific clause in the spec to be honest.
It might be worth just eating the cost of the allocation and boxing/unboxing to be honest - as you said, I expect the JIT to inline it anyway.
There was a problem hiding this comment.
It's a spec change anyway. Whether because you're synthesizing stuff that's not there, or because you change pattern matching logic.
There was a problem hiding this comment.
I did not realise that - in which case let me investigate the pattern matcher changes required and put together a proposal.
| @@ -0,0 +1,39 @@ | |||
| final class ByName1(val x: Int) extends AnyVal: | |||
| inline def isEmpty: Boolean = x == 1 | |||
There was a problem hiding this comment.
I do not believe we have a way to make this work. The JVM would probably inline these calls anyway.
3 participants
This is part of the attempt to get #19577 working without an allocation and using a by name matcher.
It attempts to correctly inline the
isEmpty,getand_Nmethods for by name matchers.The current test
by-inline.scalafailed prior to the changes the following stacktrace:Details
Looking through the code, it looks as though we delay inlining for unapply nodes.
The actual logic of which methods to call is done in the
PatternMatcherphase. However, the pattern matcher doesn't inline any calls when emitting the calls. We have a later phaseInlinePatternsto inline the body of theunapplyobject we create temporarily.My approach so far was to attempt to add the various methods in the
InlinePatternphase but I get the error below:I'm not entirely clear why the inliner doesn't "get" the owner from the inlined calls and wanted to understand whether this approach would work.
EDIT:
It seems to fail specifically when returning the literals.
Details
Or if calling a method returning the literal:
Details