Add partial cross-compilation for Scala 3 #4549

adkian-sifive · 2024-12-04T17:38:56Z

Adds initial support for Scala 3 with sources and reference to 3.3.3 LTS in the Mill build config.

This PR depends on #4467 which is (roughly) commit 0ba3c5, which are all the scala-2/ and scala/ changes visible in this PR. @jackkoenig and I need to determine whether those changes are necessary and either merge that PR first and rebase this on main, OR figure out an alternative.

I've added a CI test unit for Scala 3 compilation with Mill, @jackkoenig it probably needs your attention

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

Feature (or new API)
API modification
Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

Adds initial support for Scala 3 LTS version 3.3.3

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

.github/workflows/test.yml

build.sc

core/src/main/scala-2/chisel3/experimental/hierarchy/core/Hierarchy.scala

core/src/main/scala-3/chisel3/Aggregate.scala

core/src/main/scala-3/chisel3/Bits.scala

core/src/main/scala-3/chisel3/Disable.scala

jackkoenig · 2024-12-05T22:03:59Z

core/src/main/scala-3/chisel3/Module.scala

+  // TODO(adkian-sifive) the callsite here explicitly passes
+  // sourceInfo so it cannot be a contextual parameter


This comment looks to be no longer true? But even if it were, you can always summon[_].

so the comment here is referring to this callsite:

chisel/core/src/main/scala/chisel3/internal/package.scala

Line 133 in dc39308

Module.do_apply(new ViewParentAPI)(UnlocatableSourceInfo)

i think we need the do_apply here? (same comment for below)

That callsight should call _applyImpl, that's the replacement for do_apply. This is the same thing as the other places where we got rid of do_ versions of things because we no longer need the macro on apply, we can just take a contextual parameter.

jackkoenig · 2024-12-05T22:05:32Z

core/src/main/scala-3/chisel3/Module.scala

+  def apply[T <: BaseModule](bc: => T): T = _applyImpl(bc)
+  def do_apply[T <: BaseModule](bc: => T)(implicit sourceInfo: SourceInfo): T = apply(bc)


Suggested change

def apply[T <: BaseModule](bc: => T): T = _applyImpl(bc)

def do_apply[T <: BaseModule](bc: => T)(implicit sourceInfo: SourceInfo): T = apply(bc)

def apply[T <: BaseModule](bc: => T)(using SourceInfo): T = _applyImpl(bc)

No do_apply and apply needs the context parameter.

core/src/main/scala-3/chisel3/Printf.scala

This removes much of the use of this.type in Chisel which is necessary to upgrade to Scala 3.

Cross compilation errors otherwise

This reverts commit 0ba3c5d.

This simplifies build.sc handling of plugins

Update plugin dependency handling for Scala3

Format code

Use compileAll in CI Mill compilation

Share def format

Refactor cross-compilation logic

core/src/main/scala/chisel3/ChiselEnumImpl.scala

jackkoenig · 2024-12-18T22:33:40Z

core/src/main/scala-3/chisel3/Module.scala

+  def apply[T <: BaseModule](bc:    => T): T = _applyImpl(bc)
+  def do_apply[T <: BaseModule](bc: => T)(implicit sourceInfo: SourceInfo): T = apply(bc)


Suggested change

def apply[T <: BaseModule](bc: => T): T = _applyImpl(bc)

def do_apply[T <: BaseModule](bc: => T)(implicit sourceInfo: SourceInfo): T = apply(bc)

def apply[T <: BaseModule](bc: => T)(using sourceInfo: SourceInfo): T = _applyImpl(bc)

As mentioned above, change that Module.do_apply call in chisel/core/src/main/scala/chisel3/internal/package.scala to use _applyImpl.

so making this change + changing the Module.do_apply to Module._applyImpl in internal/package.scala throws this error

core/src/main/scala/chisel3/internal/package.scala:133:12: method _applyImpl in trait ObjectModuleImpl cannot be accessed as a member of object chisel3.Module from package object internal in package internal [error] Access to protected method _applyImpl not permitted because [error] enclosing package object internal in package internal is not a subclass of [error] trait ObjectModuleImpl in package chisel3 where target is defined [error] Module._applyImpl(new ViewParentAPI)(UnlocatableSourceInfo) [error] ^

not really sure what subclassing requirement is not being satisfied here

i'm guessing it's cause _applyImpl is protected in ObjectModuleImpl?

Right, try making it protected[chisel3] instead

jackkoenig · 2024-12-18T22:36:31Z

core/src/main/scala-3/chisel3/PrintfMacros.scala

+object PrintfMacrosCompat {
+  private[chisel3] def printfWithReset(
+    pable: Printable
+  )(
+    using sourceInfo: SourceInfo
+  ): chisel3.printf.Printf = {
+    var printfId: chisel3.printf.Printf = null
+    when(!Module.reset.asBool) {
+      printfId = printfWithoutReset(pable)
+    }
+    printfId
+  }
+
+  private[chisel3] def printfWithoutReset(
+    pable: Printable
+  )(
+    using sourceInfo: SourceInfo
+  ): chisel3.printf.Printf = {
+    val clock = Builder.forcedClock
+    val printfId = new chisel3.printf.Printf(pable)
+
+    Printable.checkScope(pable)
+
+    pushCommand(chisel3.internal.firrtl.ir.Printf(printfId, sourceInfo, clock.ref, pable))
+    printfId
+  }
+}


Wy is all of this code here instead of in src/main/scala? This is definitely code that we don't wan't duplicated as it's likely to need to change and that will cause bugs. There's an open PR touching similar code: #4504.

jackkoenig · 2024-12-18T22:36:53Z

core/src/main/scala-3/chisel3/VerificationStatementMacros.scala

+  def formatFailureMessage(
+    kind:     String,
+    lineInfo: SourceLineInfo,
+    cond:     Bool,
+    message:  Option[Printable]
+  )(
+    using sourceInfo: SourceInfo
+  ): Printable = {
+    val (filename, line) = lineInfo
+    val lineMsg = s"$filename:$line".replaceAll("%", "%%")
+    message match {
+      case Some(msg) =>
+        p"$kind failed: $msg\n"
+      case None => p"$kind failed at $lineMsg\n"
+    }


Why is this duplicated?

jackkoenig · 2024-12-18T22:39:01Z

core/src/main/scala-3/chisel3/experimental/hierarchy/IsA.scala

+  // This code handles a special-case where, within an mdoc context, the type returned from
+  //  scala reflection (typetag) looks different than when returned from java reflection.
+  //  This function detects this case and reshapes the string to match.
+  private def modifyReplString(clz: String): String = {
+    if (clz != null) {
+      clz.split('.').toList match {
+        case "repl" :: "MdocSession" :: app :: rest => s"$app.this." + rest.mkString(".")
+        case other                                  => clz
+      }
+    } else clz
+  }
+


This method should be in src/main/scala

core/src/main/scala/chisel3/BitsImpl.scala

core/src/main/scala/chisel3/experimental/hierarchy/InstantiateImpl.scala

jackkoenig

There's much more to do and I'm sure there will be issues we need to solve, but this is a huge step! Great work @adkian-sifive

FYI @sequencer

sequencer · 2024-12-19T22:47:51Z

Extraordinary work!
BTW, I’m trying to figure out a new types system and Scala 3 only design pattern and ask for public review before next Q1.

adkian-sifive added Feature New feature, will be included in release notes API Modification Scala 3 Changes related to upgrading to Scala 3 and removed API Modification labels Dec 4, 2024

adkian-sifive force-pushed the adkian-sifive/scala3-v0.1 branch from fb521df to 725a184 Compare December 4, 2024 17:50

jackkoenig reviewed Dec 5, 2024

View reviewed changes

adkian-sifive force-pushed the adkian-sifive/scala3-v0.1 branch 3 times, most recently from b07dc4a to 30d797f Compare December 12, 2024 01:47

adkian-sifive and others added 21 commits December 18, 2024 02:00

Add Scala3 sources

f4b01bc

Update build.sc

cbaeaa5

Fix SerializableModuleGenerator

fa910dc

Fix D/I sources to cross compile

88c18b6

Move SerializableModuleGenerator

c8a1fcc

Remove copy method from IR

fe3a9a7

Explicit import in ModuleImpl

cb866e2

Cross compilation fixes

8103a01

Convert cloneType to an extension method

dbd96b7

This removes much of the use of this.type in Chisel which is necessary to upgrade to Scala 3.

Updates

a5afb37

Add missing return type to unary_~

416cdda

Remove do-apply pattern from Definition and Instance

182770d

Rebase fixes

3fb5779

Switch Scala3 sourceInfo implicits to contextual parameters

65d04a1

Add Scala 3 compilation to CI

8d32700

Update workflows with explicit scala version

365e084

Cross compilation errors otherwise

CI and build.sc updates

cee3eea

Revert "Convert cloneType to an extension method"

2f3df15

This reverts commit 0ba3c5d.

Get things working with this.type

c0fe7bb

Move plugins to plugin/src/main/scala-2

65f87c1

This simplifies build.sc handling of plugins

Add plugin 3.3.4 stub and change version to 3.3.4

5763b57

Update plugin dependency handling for Scala3

adkian-sifive added 4 commits December 18, 2024 02:00

Code review updates

04821b7

Format code

Add compileAll function in build.sc

77a10f4

Use compileAll in CI Mill compilation

Update Printf.scala

e694a6a

Share def format

Hierarchy updates

88cb4d3

Refactor cross-compilation logic

adkian-sifive force-pushed the adkian-sifive/scala3-v0.1 branch from 3f5ca9d to 88cb4d3 Compare December 18, 2024 10:09

Fix Instance.scala

bc6ff61

jackkoenig reviewed Dec 18, 2024

View reviewed changes

core/src/main/scala/chisel3/ChiselEnumImpl.scala Outdated Show resolved Hide resolved

adkian-sifive force-pushed the adkian-sifive/scala3-v0.1 branch from a6046d8 to 0f72720 Compare December 18, 2024 20:07

Fix ChiselEnum

9a8b282

adkian-sifive force-pushed the adkian-sifive/scala3-v0.1 branch from 0f72720 to 9a8b282 Compare December 18, 2024 20:11

Add remaining units to buildUnits build.sc

f46d276

jackkoenig reviewed Dec 18, 2024

View reviewed changes

core/src/main/scala/chisel3/BitsImpl.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Dec 18, 2024

View reviewed changes

core/src/main/scala/chisel3/experimental/hierarchy/InstantiateImpl.scala Outdated Show resolved Hide resolved

adkian-sifive and others added 3 commits December 19, 2024 12:22

Code review updates

dfd3245

Code review updates

d754f9f

Merge branch 'main' into adkian-sifive/scala3-v0.1

8ccc39c

jackkoenig changed the title ~~Add initial support for Scala 3~~ Add partial cross-compilation for Scala 3 Dec 19, 2024

jackkoenig added this to the 7.0 milestone Dec 19, 2024

jackkoenig enabled auto-merge (squash) December 19, 2024 22:27

jackkoenig approved these changes Dec 19, 2024

View reviewed changes

jackkoenig merged commit 2d9c719 into main Dec 19, 2024
15 checks passed

jackkoenig deleted the adkian-sifive/scala3-v0.1 branch December 19, 2024 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add partial cross-compilation for Scala 3 #4549

Add partial cross-compilation for Scala 3 #4549

adkian-sifive commented Dec 4, 2024 •

edited by jackkoenig

Loading

jackkoenig Dec 5, 2024

adkian-sifive Dec 17, 2024

jackkoenig Dec 18, 2024

jackkoenig Dec 5, 2024

jackkoenig Dec 18, 2024

adkian-sifive Dec 19, 2024

adkian-sifive Dec 19, 2024

adkian-sifive Dec 19, 2024

jackkoenig Dec 19, 2024

jackkoenig Dec 18, 2024

jackkoenig Dec 18, 2024

jackkoenig Dec 18, 2024

jackkoenig left a comment

sequencer commented Dec 19, 2024

		// TODO(adkian-sifive) the callsite here explicitly passes
		// sourceInfo so it cannot be a contextual parameter

		def apply[T <: BaseModule](bc: => T): T = _applyImpl(bc)
		def do_apply[T <: BaseModule](bc: => T)(implicit sourceInfo: SourceInfo): T = apply(bc)

	def apply[T <: BaseModule](bc: => T): T = _applyImpl(bc)
	def do_apply[T <: BaseModule](bc: => T)(implicit sourceInfo: SourceInfo): T = apply(bc)
	def apply[T <: BaseModule](bc: => T)(using SourceInfo): T = _applyImpl(bc)

Add partial cross-compilation for Scala 3 #4549

Add partial cross-compilation for Scala 3 #4549

Conversation

adkian-sifive commented Dec 4, 2024 • edited by jackkoenig Loading

Contributor Checklist

Type of Improvement

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

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

jackkoenig left a comment

Choose a reason for hiding this comment

sequencer commented Dec 19, 2024

adkian-sifive commented Dec 4, 2024 •

edited by jackkoenig

Loading