Skip to content

Attempt to inline by name matcher methods #19631

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Attempt to inline by name matcher methods #19631

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b181d4
Attempt to inline by name matcher methods
yilinwei Feb 6, 2024
4c36dc1
Adding extra passing case.
yilinwei Feb 6, 2024
db13203
Add last failing case.
yilinwei Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/InlinePatterns.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package dotc
package transform

import core.*
import inlines.Inlines
import MegaPhase.*
import Symbols.*, Contexts.*, Types.*, Decorators.*
import Symbols.*, Contexts.*, Types.*, Decorators.*, StdNames.*
import Flags.*
import NameOps.*
import Names.*

Expand Down Expand Up @@ -36,7 +38,14 @@ class InlinePatterns extends MiniPhase:
// by the pattern matcher but are still not visible in that group of phases.
override def runsAfterGroupsOf: Set[String] = Set(PatternMatcher.name)

override def transformApply(app: Apply)(using Context): Tree =
override def transformSelect(sel: Select)(using Context): Tree =
if PatternMatcher.isPatmatGenerated(sel) && sel.symbol.is(Inline) then
val tree = Inlines.inlineCall(sel)
report.log(i"inline $sel -> $tree")
tree
else sel

override def transformApply(app: Apply)(using Context): Tree = {
if app.symbol.name.isUnapplyName && !app.tpe.isInstanceOf[MethodicType] then
app match
case App(Select(fn, name), argss) =>
Expand All @@ -46,6 +55,7 @@ class InlinePatterns extends MiniPhase:
case _ =>
app
else app
}

private object App:
def unapply(app: Tree): (Tree, List[List[Tree]]) =
Expand Down
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/Inlining.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import core.*
import Flags.*
import Contexts.*
import Symbols.*
import StdNames.*

import dotty.tools.dotc.ast.tpd
import dotty.tools.dotc.ast.Trees.*
Expand Down Expand Up @@ -44,8 +45,12 @@ class Inlining extends MacroTransform, IdentityDenotTransformer {
tree match {
case PackageDef(pid, _) if tree.symbol.owner == defn.RootClass =>
new TreeTraverser {

def traverse(tree: Tree)(using Context): Unit =
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work. Everything must be inlined after this stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@yilinwei yilinwei Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a spec change anyway. Whether because you're synthesizing stuff that's not there, or because you change pattern matching logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not realise that - in which case let me investigate the pattern matcher changes required and put together a proposal.

case tree: RefTree if !Inlines.inInlineMethod && StagingLevel.level == 0 =>
assert(!tree.symbol.isInlineMethod, tree.show)
case _ =>
Expand Down
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ object PatternMatcher {
def isPatmatGenerated(sym: Symbol)(using Context): Boolean =
sym.is(Synthetic) && sym.name.is(PatMatStdBinderName)

def isPatmatGenerated(select: Select)(using Context): Boolean = select match {
case Select(sel, nme.isEmpty | nme.get) if isPatmatGenerated(sel.symbol) => true
case _ => false
}

/** The pattern matching translator.
* Its general structure is a pipeline:
*
Expand Down
11 changes: 11 additions & 0 deletions tests/pos/byname-inline.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
final class ByName1(val x: Int) extends AnyVal:
inline def isEmpty: Boolean = false
Copy link
Contributor Author

@yilinwei yilinwei Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

inline def get: String = x.toString

object ByName1:
inline def unapply(x: Int): ByName1 = new ByName1(x)

def useByNamePatMatch: String =
val x = 1
x match
case ByName1(s) => s