Skip to content
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.

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

Redefine quoted.Expr.betaReduce #9469

Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Jul 30, 2020

Redefine quoted.Expr.betaRduce to not rely on complex type level computations.
Changed the signature as follows

- type TupleOfExpr[Tup <: Tuple] = Tuple.Map[Tup, [X] =>> QuoteContext ?=> Expr[X]]
- def betaReduce[F, Args <: Tuple, R, G](f: Expr[F])(using tf: TupledFunction[F, Args => R], tg: TupledFunction[G, TupleOfExpr[Args] => Expr[R]], qctx: QuoteContext): G = ...
- def betaReduceGiven[F, Args <: Tuple, R, G](f: Expr[F])(using tf: TupledFunction[F, Args ?=> R], tg: TupledFunction[G, TupleOfExpr[Args] => Expr[R]], qctx: QuoteContext): G = ...
+ def betaReduce[T](expr: Expr[T])(using qctx: QuoteContext): Expr[T] = ...

Improvements

  • Simpler API that covers all kind of functions at once (normal/given/erased)
  • Better error message for ill-typed betaRduce calls
  • Adds the possiblility of knowing if the beta-reeduction suceeded
  • Use transform.BetaReduce
  • Improve transform.BetaReduce to handle Inlined trees and constant argumets

Drawback

  • Need for slightly loneger code (previous interface could be implented on top of this one)

Expr.betaReduce(fn)(arg)
def betaReduceImpl[Arg: Type, Result: Type](fn: Expr[Arg=>Result])(arg: Expr[Arg])(using qctx : QuoteContext): Expr[Result] =
val app = '{$fn($arg)}
Expr.betaReduce(app).getOrElse(app)
Copy link
Member

Choose a reason for hiding this comment

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

If the most common usecase is to do betaReduce(x).getOrElse(x), wouldn't it be nicer to make betaReduce return its input in case it couldn't do any beta-reduction? It would still be possible to explicitly check if it did something by comparing the input and output using eq

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that might be simpler to use. I will try it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@nicolasstucki nicolasstucki force-pushed the rework-quoted-expr-beta-reduce branch from 34d2587 to 89c6ac6 Compare July 30, 2020 14:36
Redefine `quoted.Expr.betaRduce` to not rely on complex type level computations.
Changed the signature as follows
```diff
- def betaReduce[F, Args <: Tuple, R, G](f: Expr[F])(using tf: TupledFunction[F, Args => R], tg: TupledFunction[G, TupleOfExpr[Args] => Expr[R]], qctx: QuoteContext): G = ...
+ def betaReduce[T](expr: Expr[T])(using qctx: QuoteContext): Expr[T] = ...
```

Improvements
* Simpler API that covers all kind of functions at once (normal/given/erased)
* Better error message for ill-typed `betaRduce` calls
* Adds the possiblility of knowing if the beta-reeduction suceeded
* Use `transform.BetaReduce`
* Improve `transform.BetaReduce` to handle `Inlined` trees and constant argumets
* Fixes scala#9466

Drawback
* Need for slightly loneger code (previous interface could be implented on top of this one)
@nicolasstucki nicolasstucki force-pushed the rework-quoted-expr-beta-reduce branch from 89c6ac6 to 78f6ee4 Compare July 30, 2020 17:58
@nicolasstucki nicolasstucki marked this pull request as ready for review July 31, 2020 05:30
@nicolasstucki
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9469/ to see the changes.

Benchmarks is based on merging with master (316d218)

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM

@@ -65,7 +69,8 @@ object BetaReduce:
ref.symbol
case _ =>
val flags = Synthetic | (param.symbol.flags & Erased)
val binding = ValDef(newSymbol(ctx.owner, param.name, flags, arg.tpe.widen, coord = arg.span), arg)
val tpe = if arg.tpe.dealias.isInstanceOf[ConstantType] then arg.tpe.dealias else arg.tpe.widen
val binding = ValDef(newSymbol(ctx.owner, param.name, flags, tpe, coord = arg.span), arg).withSpan(arg.span)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unclear to me why we cannot just use arg.tpe.widen as the type of the binding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is to allow TermRef to definitions with constant types to be constant folded after this transformation.

override def transform(tree: Tree)(using Context) = tree.tpe.widenTermRefExpr match
case ConstantType(const) if isPureExpr(tree) => cpy.Literal(tree)(const)
case _ => super.transform(tree)
}.transform(expansion)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is constant folding -- can we reuse the logic in ConstFold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not exactly constant folding. It is just the propagation of constants. After this ConstFold is used at some point and performs the actual folding.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about TreeInfo.constToLiteral?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fails. It also looks like overkill.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Maybe as an optimization for later, beta-reduce & inlining may create new const-folding opportunities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed

library/src-bootstrapped/scala/quoted/Expr.scala Outdated Show resolved Hide resolved
Co-authored-by: Fengyun Liu <liu@fengy.me>
@nicolasstucki nicolasstucki merged commit 0655346 into scala:master Aug 3, 2020
@nicolasstucki nicolasstucki deleted the rework-quoted-expr-beta-reduce branch August 3, 2020 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inline method stops inlining function when called naming the parameter
4 participants