🔥 ข่าวด่วน: scala/scala/pull/

SolalPirelli · 2026-03-10T11:02:16Z

Found while investigating #24970 but unfortunately does not fix that problem.

How much have your relied on LLM-based tools in this contribution?

not

How was the solution tested?

existing tests

SolalPirelli · 2026-03-10T11:38:01Z

+          if typerPhase.period == Periods.InvalidPeriod then
+            getMember(owner, innerName.toTypeName)
+          else
+            atPhase(typerPhase)(getMember(owner, innerName.toTypeName))


⚠️ bugfix, this is not great, but it's better than doing atPhase with an uninitialized phase.

SolalPirelli · 2026-03-10T11:38:26Z

     */
    def initial: SingleDenotation =
-      if (validFor.firstPhaseId <= 1) this
+      if (validFor.firstPhaseId == FirstPhaseId) this


cannot be == 0 here unless something has gone terribly wrong

SolalPirelli · 2026-03-10T11:38:34Z

     */
    def skipRemoved(using Context): SingleDenotation =
-      if (validFor.code <= 0) nextDefined else this
+      if (validFor == Nowhere) nextDefined else this


code is nonnegative

SolalPirelli · 2026-03-10T11:38:43Z

      end goBack

-      if valid.code <= 0 then
+      if valid == Nowhere then


code is nonnegative

SolalPirelli · 2026-03-10T11:38:54Z

   *     last phase id:        7 bits
   *     #phases before last:  7 bits
   *
-   *     // Dmitry: sign == 0 isn't actually always true, in some cases phaseId == -1 is used for shifts, that easily creates code < 0


no longer true

SolalPirelli · 2026-03-10T11:39:30Z

-          case InitialPeriod                     => "InitialPeriod"
-          case InvalidPeriod                     => "InvalidPeriod"
-          case Period(NoRunId, 0, PhaseMask)     => s"Period(NoRunId.all)"
-          case Period(runId, 0, PhaseMask)       => s"Period($runId.all)"


this + the next few diffs: no reason to use 0 as the first phase ID to say all when we already have a constant defined to mean that

also, PhaseMask is logically the wrong thing here, even though it's equivalent to MaxPossiblePhaseId

SolalPirelli · 2026-03-10T11:40:13Z

    }
  }

-  inline val NowhereCode = 0


moved down so earlier vals don't depend on later ones

SolalPirelli · 2026-03-10T11:40:30Z

 import scala.collection.mutable.ListBuffer
 import dotty.tools.dotc.transform.MegaPhase.*
 import dotty.tools.dotc.transform.*
-import Periods.*


duplicate import with line 5

SolalPirelli · 2026-03-10T11:41:19Z

+            && ctx.phaseId <= denot.validFor.lastPhaseId
            && lastDenot != null
-            && lastDenot.validFor.lastPhaseId > denot.validFor.firstPhaseId
+            && denot.validFor.firstPhaseId < lastDenot.validFor.lastPhaseId


⚠️ bugfix, plus rewrite of the expr to be consistent between the checks: before this, we didn't have the second check, and CC would end up creating periods with an end phase before the start phase

Consider adding a def containsPhaseIdNotFirst(id: PhaseId): Boolean in Period to perform the two first tests. This way it stays close to containsPhaseId and any optimization can be replicated to the other one.

SolalPirelli · 2026-03-10T11:42:08Z

+  given runContext[Dummy_so_its_a_def]: Context =
+    if myCtx eq null then myCtx = rootContext(using ictx)
+    assert(myCtx.nn.runId <= Periods.MaxPossibleRunId)
+    myCtx.nn


⚠️ bugfix, before this reset would leave the run in an invalid state since it sets myCtx to null but there was no code to re-initialize it (in practice we never actually reuse a run that we've reset so this didn't cause problems)

I assume this is not a perf-sensitive method?

I hope not, but also I don't really know why this assertion is there, it could probably just be deleted

ok so it is perf sensitive, I reverted this change... too bad

SolalPirelli · 2026-03-10T11:42:52Z

+      this.code < that.code
+
+    inline def >(that: Period): Boolean =
+      this.code > that.code


I didn't dare make these virtual methods in any way because of perf concerns, but I also don't think any code outside of Period should be accessing its .code

SolalPirelli · 2026-03-11T14:42:28Z

Found even more bugs, will put back in draft until those are fixed + I'm running benchmarks to make sure this doesn't kill perf

SolalPirelli · 2026-03-11T15:30:11Z

@@ -0,0 +1,40 @@
+package dotty.tools.dotc.core


noticed there was a worksheet testing Period, so I ported it to actual tests

See also my comment on exhaustively testing Period.contains (obviously not on every CI run, but as a script that we can manually run when needed).

SolalPirelli · 2026-03-11T15:31:10Z

-      //      lastDiff + d2 <= d1
-      //  iff X == 0 && l1 - l2 >= 0 && l1 - l2 + d2 <= d1
-      //  iff r1 == r2 & l1 >= l2 && l1 - d1 <= l2 - d2
-      //  q.e.d


⚠️ this proof was wrong, counter-example this = (1, 56, 1), that = (1, 51, 32), found by Z3 with:

(declare-fun r1 () (_ BitVec 17)) (declare-fun l1 () (_ BitVec 7)) (declare-fun d1 () (_ BitVec 7)) (declare-fun r2 () (_ BitVec 17)) (declare-fun l2 () (_ BitVec 7)) (declare-fun d2 () (_ BitVec 7)) ; d <= l (assert (bvule d1 l1)) (assert (bvule d2 l2)) ; l < 64 (assert (bvult l1 #b1000000)) (assert (bvult l2 #b1000000)) ; r > 0 (assert (bvult #b00000000000000000 r1)) (assert (bvult #b00000000000000000 r2)) (declare-fun code1 () (_ BitVec 32)) (assert (= code1 (concat #b0 r1 l1 d1))) (declare-fun code2 () (_ BitVec 32)) (assert (= code2 (concat #b0 r2 l2 d2))) (declare-fun phaseWidth () (_ BitVec 32)) (declare-fun phaseMask () (_ BitVec 32)) (assert (= phaseWidth #x00000007)) (assert (= phaseMask #x0000007F)) (declare-fun lastDiff () (_ BitVec 32)) (assert (= lastDiff (bvlshr (bvsub code1 code2) phaseWidth))) (declare-fun result () Bool) (assert (= result (bvsle (bvadd lastDiff (bvand code1 phaseMask)) (bvand code2 phaseMask)))) (declare-fun ideal () Bool) (assert (= ideal (and (= r1 r2) (bvuge l1 l2) (bvule (bvsub l1 d1) (bvsub l2 d2))))) (assert (not (= result ideal))) (check-sat) (get-model)

Does that mean you have a Z3 proof that the new code is correct? If yes, add it in a big comment somewhere, so we can retest it in the future if necessary?

SolalPirelli · 2026-03-11T15:31:41Z

    def runId: RunId = code >>> (PhaseWidth * 2)

-    /** The phase identifier of this single-phase period. */
-    def phaseId: PhaseId = (code >>> PhaseWidth) & PhaseMask


⚠️ this was used on periods with more than one phase... but in practice it seems it's used as a "first phase ID", so I modified callers to just use that

SolalPirelli · 2026-03-13T09:38:38Z

This latest version is within 2% of main, on each side, i.e., sometimes a little faster and sometimes a little slower: https://lampepfl.github.io/scala3-benchmarks/#compare/3.8.4-RC1-bin-5c4fcc611d04bba02ef9d03645f4b60462c6fff3-BENCH,3.8.4-RC1-bin-877fc2cb695bffb63abe6302b58c6f0f7ecb29e2-BENCH

Given that main's code isn't actually correct, I think that's pretty good.

SolalPirelli · 2026-03-13T13:15:12Z

    def recordRecheckPhase(phase: Recheck): Unit =
      val id = phase.id
-      assert(id < 64, s"Recheck phase with id $id outside permissible range 0..63")
+      assert(id < 64, s"Recheck phase with id $id outside range 0..63, cannot use Long bits encoding")


just to make it clear this is an invariant of Phases, not of periods

sjrd · 2026-03-19T15:23:00Z

-      //      lastDiff + d2 <= d1
-      //  iff X == 0 && l1 - l2 >= 0 && l1 - l2 + d2 <= d1
-      //  iff r1 == r2 & l1 >= l2 && l1 - d1 <= l2 - d2
-      //  q.e.d


Does that mean you have a Z3 proof that the new code is correct? If yes, add it in a big comment somewhere, so we can retest it in the future if necessary?

sjrd · 2026-03-19T15:25:53Z

+  private inline val NowhereCode = 0
+  final val Nowhere: Period = new Period(NowhereCode)
+  final val InitialPeriod: Period = Period(InitialRunId, FirstPhaseId)
+  final val InvalidPeriod: Period = Period(NoRunId, NoPhaseId)


What's the difference between Nowhere and InvalidPeriod? IIUC they both end up with code == 0.

Good point. InvalidPeriod only had 3 uses so I replaced it with Nowhere.

sjrd · 2026-03-19T15:31:39Z


    /** The first phase of this period */
-    def firstPhaseId: Int = lastPhaseId - (code & PhaseMask)
+    def firstPhaseId: PhaseId = lastPhaseId - (code & PhaseMask)


Could be done with one fewer operation:

((code >>> PhaseWidth) - code) & PhaseMask

(not sure it's really a desirable change, but if you were after raw perf numbers, it might)

sjrd · 2026-03-19T15:46:17Z

-    def firstPhaseId: Int = lastPhaseId - (code & PhaseMask)
+    def firstPhaseId: PhaseId = lastPhaseId - (code & PhaseMask)

    def containsPhaseId(id: PhaseId): Boolean = firstPhaseId <= id && id <= lastPhaseId


Probably an actual perf improvement here. Use the property that if a <= b, then a <= x && x <= b is equivalent to (x - a) unsigned_<= (b - a). Here a == firstPhaseId == lastP - diff and b == lastP. So
(x - a) == x - (lastP - diff) == x + diff - lastP
(b - a) == lastP - (lastP - diff) == diff
Hence:

Suggested change

def containsPhaseId(id: PhaseId): Boolean = firstPhaseId <= id && id <= lastPhaseId

def containsPhaseId(id: PhaseId): Boolean =

val diff = code & PhaseMask

((id + diff - lastPhaseId) ^ Int.MinValue) <= (diff ^ Int.MinValue)

where (x ^ MinValue) <= (y ^ MinValue) is the encoding of x unsigned_<= y.

sjrd · 2026-03-19T15:47:01Z

-    }
+    /** Does this period contain the given period? */
+    def contains(that: Period): Boolean =
+      // We want to check (run1 == run2) & (last1 >= last2) & (first1 <= first2).


Put this formula in the publicly visible Scaladoc?

sjrd · 2026-03-19T15:57:31Z

+      // in the sign + run ID bits. If (run1 > run2) or (run2 < run1) or (run1 == run2 and last1 < last2),
+      // then (code1 - code2) is either large enough to have some run ID bits set, or negative so the sign bit is set.
+      ((this.code - that.code) & (-1 << (PhaseWidth * 2))) == 0 &&
+        this.firstPhaseId <= that.firstPhaseId


The second line is equivalent to that.firstPhaseId - this.firstPhaseId >= 0, which is equivalent to ((that.firstPhaseId - this.firstPhaseId) & SignBit) == 0.

So now you have (expr1 == 0) && (expr2 == 0), which is equivalent to (expr1 | expr2) == 0. Which means you can do everything with a single test:

(((this.code - that.code) & (-1 << (PhaseWidth * 2))) | ((that.firstPhaseId - this.firstPhaseId) & Int.MinValue)) == 0

I suggest exhaustively testing the formula we choose. We can limit choices of runId to two bits (from 0 to 3), and exhaustively test all possible values of lastPhaseId and diff. That's only 16 bits per operand, hence 32 bits of input. Testing the equivalence of two functions on 2^32 inputs can be done in a few minutes (ok maybe a few dozens of minutes, but definitely less than an hour).

yeah it turns out I made a silly typo in one of my hex constants when originally writing this, and I'm wrong.

It's possible for run1 != run2 yet code1 - code2 has no run ID bits set and isn't negative either, e.g.:

0 00000100100000001 0111110 0000000 - 0 00000100100000000 0111111 0000001 = 0 00000000000000000 1111110 1111111

Ah yes. Carry gets canceled out.

I guess we can prevent that by sacrificing the bit n°14 (forcing it to always 0). We would have 1 fewer bit available to encode runIds, but then this would not happen anymore.

I'll run benchmarks on this PR over the weekend, so we can decide if halving the number of run IDs is needed

sjrd · 2026-03-19T16:06:49Z

+            && ctx.phaseId <= denot.validFor.lastPhaseId
            && lastDenot != null
-            && lastDenot.validFor.lastPhaseId > denot.validFor.firstPhaseId
+            && denot.validFor.firstPhaseId < lastDenot.validFor.lastPhaseId


Consider adding a def containsPhaseIdNotFirst(id: PhaseId): Boolean in Period to perform the two first tests. This way it stays close to containsPhaseId and any optimization can be replicated to the other one.

sjrd · 2026-03-19T16:09:22Z

+  given runContext[Dummy_so_its_a_def]: Context =
+    if myCtx eq null then myCtx = rootContext(using ictx)
+    assert(myCtx.nn.runId <= Periods.MaxPossibleRunId)
+    myCtx.nn


I assume this is not a perf-sensitive method?

sjrd · 2026-03-19T16:10:11Z

@@ -0,0 +1,40 @@
+package dotty.tools.dotc.core


See also my comment on exhaustively testing Period.contains (obviously not on every CI run, but as a script that we can manually run when needed).

sjrd · 2026-03-19T16:12:40Z

        def computeMemberTpe(): Type =
          if (sym.is(Method)) sym.denot.info
-          else if sym.denot.validFor.phaseId > erasurePhase.id && sym.isField && sym.getter.exists then
+          else if sym.denot.validFor.firstPhaseId > erasurePhase.id && sym.isField && sym.getter.exists then


Some .phaseIds become .firstPhaseId, and others became lastPhaseId. Elaborate?

here "entered after erasure" in the comment right below so it makes sense to look at firstPhaseId I think

I think my reasoning was "set to lastPhaseId which is cheaper, unless in context firstPhaseId makes more sense"

som-snytt · 2026-03-19T16:26:42Z

As a performance footnote, I noticed 5f390be for inlined contains test.

SolalPirelli · 2026-03-20T14:11:00Z

@som-snytt I don't think this is true anymore, a denot can have the period of a phase whose period has more than one phase because it's a list of mini-phases

SolalPirelli · 2026-03-20T14:24:24Z

    final def phase: Phase = base.phases(period.firstPhaseId)
    final def runId = period.runId
-    final def phaseId = period.phaseId
+    final def phaseId = period.firstPhaseId


It feels wrong for this one to be lastPhaseId given the def phase using firstPhaseId just two lines above

mbovel · 2026-03-23T12:16:19Z

Benchmarks started. Workflow run.

mbovel · 2026-03-23T23:09:08Z

Benchmarks completed. Overview.

SolalPirelli · 2026-03-24T08:00:36Z

@sjrd latest subset of benchmarks: https://lampepfl.github.io/scala3-benchmarks/#compare/3.8.4-RC1-bin-20260323-16cd4d9-NIGHTLY,3.8.4-RC1-bin-ad72f3091dc04606025b12b8e8a039094d97b22e-BENCH

Not sure if within noise or not. I could try the "steal a bit from run IDs" tactic, but maybe that's better left in a separate PR since at least it's correct now?

mbovel · 2026-03-24T08:58:48Z

Not sure if within noise or not.

If you're unsure, feel free to run them again; the results will aggregate automatically.

sjrd · 2026-03-30T13:43:29Z

+
+  private inline val NowhereCode = 0
+  final val Nowhere: Period = new Period(NowhereCode)
+  final val InitialPeriod: Period = Period(InitialRunId, FirstPhaseId)


This is not as good as one might think. Only primitive types can be inlined as inline vals. For custom value classes, it doesn't work.

If you don't need the stability, you can reclaim the earlier best code by making them inline defs instead.

they're used in pattern matching so we need stability:

Error: -- [E135] Type Error: /home/runner/work/scala3/scala3/compiler/src/dotty/tools/dotc/core/Periods.scala:170:15 Error: 170 | case Nowhere => "Nowhere" Error: | ^^^^^^^ Error: |Stable identifier required, but dotty.tools.dotc.core.Periods.Nowhere found

that being said, they were already final val before, so this hasn't changed...?

OK then leave it as before.

SolalPirelli commented Mar 10, 2026

View reviewed changes

SolalPirelli requested a review from sjrd March 10, 2026 17:57

SolalPirelli marked this pull request as ready for review March 10, 2026 17:57

SolalPirelli force-pushed the solal/harden-period branch from 96a7fe5 to e622291 Compare March 11, 2026 14:41

SolalPirelli marked this pull request as draft March 11, 2026 14:42

SolalPirelli commented Mar 11, 2026

View reviewed changes

SolalPirelli force-pushed the solal/harden-period branch from d465112 to 877fc2c Compare March 12, 2026 16:48

SolalPirelli marked this pull request as ready for review March 13, 2026 13:13

SolalPirelli commented Mar 13, 2026

View reviewed changes

SolalPirelli force-pushed the solal/harden-period branch 2 times, most recently from d77df66 to a7308cb Compare March 18, 2026 08:07

sjrd requested changes Mar 19, 2026

View reviewed changes

SolalPirelli commented Mar 20, 2026

View reviewed changes

SolalPirelli force-pushed the solal/harden-period branch from a7308cb to a4e4cf9 Compare March 20, 2026 14:39

SolalPirelli force-pushed the solal/harden-period branch from a4e4cf9 to 6d9819d Compare March 20, 2026 14:40

SolalPirelli added 9 commits March 23, 2026 09:32

Harden Period creation and use of Run/PhaseId

befcd91

Fix Period.contains; counter-ex: this=(1,56,1) that=(1,51,32)

0b53e29

Store first phase ID directly

80689c8

Delete Period.phaseId, force explicit use of first/last

c5545eb

Perf?

b4a4938

More perf?

67277dc

Silly me

f4789a3

PR feedback

43c274d

Let's see if Run.runContext matters

ad72f30

SolalPirelli force-pushed the solal/harden-period branch from 6d9819d to ad72f30 Compare March 23, 2026 08:32

SolalPirelli requested a review from sjrd March 24, 2026 08:00

sjrd approved these changes Mar 30, 2026

View reviewed changes

SolalPirelli commented Mar 30, 2026

View reviewed changes

Comment thread compiler/src/dotty/tools/dotc/core/Periods.scala

SolalPirelli force-pushed the solal/harden-period branch from aa92026 to ad72f30 Compare March 30, 2026 18:16

SolalPirelli marked this pull request as draft March 30, 2026 20:03

SolalPirelli marked this pull request as ready for review March 30, 2026 20:03

SolalPirelli merged commit 3da97ed into scala:main Mar 30, 2026
128 checks passed

SolalPirelli deleted the solal/harden-period branch March 30, 2026 20:04

WojciechMazur added this to the 3.8.4 milestone Mar 31, 2026

Uh oh!

Conversation

SolalPirelli commented Mar 10, 2026

How much have your relied on LLM-based tools in this contribution?

How was the solution tested?

Uh oh!

SolalPirelli Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolalPirelli commented Mar 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolalPirelli Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolalPirelli Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolalPirelli commented Mar 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SolalPirelli Mar 10, 2026 •

edited

Loading

SolalPirelli Mar 11, 2026 •

edited

Loading

SolalPirelli Mar 11, 2026 •

edited

Loading

mbovel commented Mar 24, 2026 •

edited

Loading