Merge several IR nodes into `UnaryOp`. #5088

sjrd · 2024-12-15T13:48:21Z

ArrayLength, GetClass, Clone, IdentityHashCode, WrapAsThrowable, UnwrapFromThrowable and Throw are now ops of UnaryOp, rather than having their own IR nodes.

They all follow the general contract of how a UnaryOp must behave: first evaluate the argument, then perform an operation that only depends on the value of the argument (and not, for example, on the scope).

It makes the JS FunctionEmitter a bit messier. However, everything else gets simpler, as we get rid of dedicated paths for those 7 IR nodes.

In order to better fit with the other UnaryOps, notably the ones for jl.Class operations, we demand non-nullable arguments. An explicit CheckNotNull must be used for arguments that are nullable.

In this commit, we do not update the compiler yet. It still emits the old IR nodes, so that we can test the deserialiation hacks. That also means the case classes still exist for now.

gzm0 · 2024-12-15T17:35:02Z

FWIW: IMO JSDelete and JSImportCall can benefit from a similar treatment. Did you deliberately decide against these?

sjrd · 2024-12-15T21:09:34Z

JSImportCall will need an additional argument once we add support for those with things. Also IMO it uses the scope. It's the module scope, not a function scope, but still. Its behavior is not only dependent on the argument.

For JSDelete, it's mostly because it would then be the only JS thing that we merge this way. I'd rather keep them separate. Also they have dubious behavior with evaluation order, and I don't want to pollute BinaryOp with that weirdness.

gzm0

I'd appreciate separate commits for the review changes to ease re-review.

linker/shared/src/main/scala/org/scalajs/linker/backend/wasmemitter/FunctionEmitter.scala

gzm0 · 2024-12-21T17:23:25Z

linker/shared/src/main/scala/org/scalajs/linker/backend/wasmemitter/FunctionEmitter.scala

-
-  private def genWrapAsThrowable(tree: WrapAsThrowable): Type = {
-    val WrapAsThrowable(expr) = tree
+  private def genWrapAsThrowable(tree: UnaryOp): Type = {


This seems to be inlined in genUnaryOp and unused otherwise. Remove?

gzm0 · 2024-12-21T17:25:29Z

linker/shared/src/main/scala/org/scalajs/linker/backend/wasmemitter/FunctionEmitter.scala

    }

    tree.tpe
+
+    // scalastyle:on return


Consider putting this after the early return at the beginning of the function.

IMO it will make it clearer to tell the story:

attention, this function contains early returns

code

from here on, all early returns are done.

more code

Oh that's new. But it's a good strategy, indeed!

gzm0 · 2024-12-21T17:25:57Z

linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala

@@ -801,7 +798,7 @@ private final class ClassDefChecker(classDef: ClassDef,

        checkApplyGeneric(method, args)

-      case UnaryOp(_, lhs) =>
+      case UnaryOp(op, lhs) =>


Change intended?

No. Reverted.

gzm0 · 2024-12-21T17:30:29Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/IncOptimizer.scala

@@ -882,8 +882,7 @@ final class IncOptimizer private[optimizer] (config: CommonPhaseConfig, collOps:
        case UnaryOp(UnaryOp.CheckNotNull, expr) =>
          config.coreSpec.semantics.nullPointers == CheckedBehavior.Unchecked &&
          isTriviallySideEffectFree(expr)
-        case GetClass(expr) => // Before 1.17, we used GetClass as CheckNotNull
-          config.coreSpec.semantics.nullPointers == CheckedBehavior.Unchecked &&
+        case UnaryOp(UnaryOp.GetClass, expr) => // Before 1.17, we used GetClass as CheckNotNull


If we need to keep this codepath anyways, how about:

Suggested change

case UnaryOp(UnaryOp.GetClass, expr) => // Before 1.17, we used GetClass as CheckNotNull

case UnaryOp(op, expr) if UnaryOp.isSideEffectFree(op) => // Before 1.17, we used GetClass as CheckNotNull

gzm0 · 2024-12-21T17:39:24Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

-      expandLongOps(foldUnaryOp(op, tlhs))(cont)
+      val folded = foldUnaryOp(op, tlhs)
+
+      folded match {


Is there an upside to folding the op first and then applying the checks for throwable wrapping? IIUC texpr below is always tlhs.

If not consider inverting to avoid confusion:

def folded = foldUnaryOp(op, tlhs) op match { case UnaryOp.WrapAsThrowable => if (...) else if (...) else folded // etc. case _ => expandLongOps(folded)(cont) }

No strong opinion here. I changed it to your suggestion.

gzm0 · 2024-12-21T17:43:40Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

@@ -6920,7 +6880,6 @@ private[optimizer] object OptimizerCore {
      case ApplyStatically(_, receiver, _, _, Nil) => isTrivialArg(receiver)
      case ApplyStatic(_, _, _, Nil)               => true

-      case ArrayLength(array)        => isTrivialArg(array)
      case ArraySelect(array, index) => isTrivialArg(array) && isTrivialArg(index)

      case AsInstanceOf(inner, _) => isSimpleArg(inner)


Comment should be one line lower (but Github doesn't let me).

IIUC this will consider UnwrapFromThrowable (and others) a simple arg. Not sure we want that?

Arguably we might not want it for WrapAsThrowable and UnwrapAsThrowable, but those won't actually happen in practice. Even then, at call-site there's a good chance that they can be simplified with their expansion, so it might even be better anyway.

For the other ones, IMO we do want that. One I have seen in the diffs are throws. If a method does nothing but throw a simple arg, inlining it is probably beneficial.

I did not change this in this round, but I remain open to changing it if you have a stronger opinion.

gzm0 · 2024-12-21T17:45:31Z

linker/shared/src/test/scala/org/scalajs/linker/IRCheckerTest.scala

-          ArrayLength(Null()),
-          ArraySelect(Throw(Null()), int(1))(NothingType),
-          ArrayLength(Throw(Null()))
+          UnaryOp(UnaryOp.Array_length, UnaryOp(UnaryOp.CheckNotNull, Null())),


Remove this case? IMO it doesn't test really anything anymore.

I think if you want to add a test case / adjust this, we should check that Array_length now does not allow null anymore.

gzm0 · 2024-12-21T17:46:27Z

project/Build.scala

@@ -2066,7 +2066,7 @@ object Build {
            if (!useMinifySizes) {
              Some(ExpectedSizes(
                  fastLink = 449000 to 450000,
-                  fullLink = 95000 to 96000,
+                  fullLink = 94000 to 95000,


Heh, nice :) I guess less unnecessary null checks.

project/JavaLangObject.scala

sjrd

Updated with one commit addressing the review.

project/JavaLangObject.scala

linker/shared/src/main/scala/org/scalajs/linker/backend/wasmemitter/FunctionEmitter.scala

sjrd · 2024-12-22T11:38:26Z

linker/shared/src/main/scala/org/scalajs/linker/backend/wasmemitter/FunctionEmitter.scala

    }

    tree.tpe
+
+    // scalastyle:on return


Oh that's new. But it's a good strategy, indeed!

sjrd · 2024-12-22T12:14:22Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

-      expandLongOps(foldUnaryOp(op, tlhs))(cont)
+      val folded = foldUnaryOp(op, tlhs)
+
+      folded match {


No strong opinion here. I changed it to your suggestion.

sjrd · 2024-12-22T12:19:34Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

@@ -6920,7 +6880,6 @@ private[optimizer] object OptimizerCore {
      case ApplyStatically(_, receiver, _, _, Nil) => isTrivialArg(receiver)
      case ApplyStatic(_, _, _, Nil)               => true

-      case ArrayLength(array)        => isTrivialArg(array)
      case ArraySelect(array, index) => isTrivialArg(array) && isTrivialArg(index)

      case AsInstanceOf(inner, _) => isSimpleArg(inner)


Arguably we might not want it for WrapAsThrowable and UnwrapAsThrowable, but those won't actually happen in practice. Even then, at call-site there's a good chance that they can be simplified with their expansion, so it might even be better anyway.

For the other ones, IMO we do want that. One I have seen in the diffs are throws. If a method does nothing but throw a simple arg, inlining it is probably beneficial.

I did not change this in this round, but I remain open to changing it if you have a stronger opinion.

sjrd · 2024-12-22T12:21:31Z

linker/shared/src/main/scala/org/scalajs/linker/checker/ClassDefChecker.scala

@@ -801,7 +798,7 @@ private final class ClassDefChecker(classDef: ClassDef,

        checkApplyGeneric(method, args)

-      case UnaryOp(_, lhs) =>
+      case UnaryOp(op, lhs) =>


No. Reverted.

gzm0

LGTM (after squashing obviously).

`ArrayLength`, `GetClass`, `Clone`, `IdentityHashCode`, `WrapAsThrowable`, `UnwrapFromThrowable` and `Throw` are now ops of `UnaryOp`, rather than having their own IR nodes. They all follow the general contract of how a `UnaryOp` must behave: first evaluate the argument, then perform an operation that only depends on the value of the argument (and not, for example, on the scope). It makes the JS `FunctionEmitter` a bit messier. However, everything else gets simpler, as we get rid of dedicated paths for those 7 IR nodes. In order to better fit with the other `UnaryOp`s, notably the ones for `jl.Class` operations, we demand non-nullable arguments. An explicit `CheckNotNull` must be used for arguments that are nullable. --- In this commit, we do not update the compiler yet. It still emits the old IR nodes, so that we can test the deserialiation hacks. That also means the case classes still exist for now.

And update the compiler to directly generate the new `UnaryOp`s. We only enable the deserialization hack when reading IR <= 1.17.

sjrd · 2024-12-22T14:31:29Z

(rebased to address conflicts in BinaryIncompatibilities)

And port the changes that were made to the compiler backend. Most nobably, the changes from the following upstream PRs: * Introduce non-nullable reference types in the IR. scala-js/scala-js#5018 * Opt: Remove useless *Ident indirections in the IR model. scala-js/scala-js#5092 * Merge the IR node This into VarRef, with a magic LocalName. scala-js/scala-js#5090 * Merge several IR nodes into UnaryOp. scala-js/scala-js#5088 * Restrict usages of StoreModule even further. scala-js/scala-js#5059 * Allow Return arg to be a void if the target Labeled is a void. scala-js/scala-js#5074 * Rename NoType to VoidType and print it as "void". scala-js/scala-js#5061

And port the changes that were made to the compiler backend. Most notably, the changes from the following upstream PRs: * Introduce non-nullable reference types in the IR. scala-js/scala-js#5018 * Opt: Remove useless *Ident indirections in the IR model. scala-js/scala-js#5092 * Merge the IR node This into VarRef, with a magic LocalName. scala-js/scala-js#5090 * Merge several IR nodes into UnaryOp. scala-js/scala-js#5088 * Restrict usages of StoreModule even further. scala-js/scala-js#5059 * Allow Return arg to be a void if the target Labeled is a void. scala-js/scala-js#5074 * Rename NoType to VoidType and print it as "void". scala-js/scala-js#5061

And port the changes that were made to the compiler backend. Most notably, the changes from the following upstream PRs: * scala-js/scala-js#5018 * scala-js/scala-js#5092 * scala-js/scala-js#5090 * scala-js/scala-js#5088 * scala-js/scala-js#5059 * scala-js/scala-js#5074 * scala-js/scala-js#5061

sjrd force-pushed the more-unary-ops branch from 431b9af to f54d97c Compare December 15, 2024 14:13

sjrd marked this pull request as ready for review December 15, 2024 14:21

sjrd requested a review from gzm0 December 15, 2024 14:21

sjrd force-pushed the more-unary-ops branch 2 times, most recently from 1e592a4 to 1792413 Compare December 16, 2024 09:06

sjrd mentioned this pull request Dec 21, 2024

Introduce NewLambda to synthesize instances of SAM types. #5003

Merged

sjrd force-pushed the more-unary-ops branch from 1792413 to 8c6e6e3 Compare December 21, 2024 17:59

gzm0 requested changes Dec 22, 2024

View reviewed changes

sjrd commented Dec 22, 2024

View reviewed changes

sjrd requested a review from gzm0 December 22, 2024 12:23

gzm0 approved these changes Dec 22, 2024

View reviewed changes

sjrd force-pushed the more-unary-ops branch from 0b7aa2f to 64aa028 Compare December 22, 2024 13:49

sjrd added 2 commits December 22, 2024 15:26

Remove the IR nodes that were merged in UnaryOp.

e4bdf16

And update the compiler to directly generate the new `UnaryOp`s. We only enable the deserialization hack when reading IR <= 1.17.

sjrd force-pushed the more-unary-ops branch from 798ee81 to c1646d1 Compare December 22, 2024 14:28

sjrd merged commit f82e59f into scala-js:main Dec 22, 2024
3 checks passed

sjrd deleted the more-unary-ops branch December 22, 2024 21:48

sjrd mentioned this pull request Dec 29, 2024

Drop support for Scala 2.12.{2-5}. #5095

Merged

sjrd mentioned this pull request Jan 17, 2025

Scala.js: Upgrade to Scala.js 1.18.1. scala/scala3#22397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge several IR nodes into `UnaryOp`. #5088

Merge several IR nodes into `UnaryOp`. #5088

sjrd commented Dec 15, 2024

gzm0 commented Dec 15, 2024

sjrd commented Dec 15, 2024 •

edited

Loading

gzm0 left a comment

gzm0 Dec 21, 2024

gzm0 Dec 21, 2024

sjrd Dec 22, 2024

gzm0 Dec 21, 2024

sjrd Dec 22, 2024

gzm0 Dec 21, 2024

gzm0 Dec 21, 2024

sjrd Dec 22, 2024

gzm0 Dec 21, 2024

sjrd Dec 22, 2024

gzm0 Dec 21, 2024

gzm0 Dec 21, 2024

sjrd left a comment

sjrd Dec 22, 2024

sjrd Dec 22, 2024

sjrd Dec 22, 2024

sjrd Dec 22, 2024

gzm0 left a comment

sjrd commented Dec 22, 2024

	case UnaryOp(UnaryOp.GetClass, expr) => // Before 1.17, we used GetClass as CheckNotNull
	case UnaryOp(op, expr) if UnaryOp.isSideEffectFree(op) => // Before 1.17, we used GetClass as CheckNotNull

Merge several IR nodes into UnaryOp. #5088

Merge several IR nodes into UnaryOp. #5088

Conversation

sjrd commented Dec 15, 2024

gzm0 commented Dec 15, 2024

sjrd commented Dec 15, 2024 • edited Loading

gzm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 left a comment

Choose a reason for hiding this comment

sjrd commented Dec 22, 2024

Merge several IR nodes into `UnaryOp`. #5088

Merge several IR nodes into `UnaryOp`. #5088

sjrd commented Dec 15, 2024 •

edited

Loading