diff --git a/.claude/commands/bugfix.md b/.claude/commands/bugfix.md index 7834613ed..fb5565e90 100644 --- a/.claude/commands/bugfix.md +++ b/.claude/commands/bugfix.md @@ -787,6 +787,16 @@ alias only exists in the parametric regime). Re-run the reproducer after EACH ga diagnose the remaining gates by diffing the IR shape (`getCodeString`) of the warning and non-warning twins, not by re-reading the predicate. +The gates can also sit in DIFFERENT LAYERS, one in the plugin (what gets captured) and one in +core (what gets consumed), and then the reporter's spelling of the reproducer decides which one +you see. Split them by varying the DECLARATION KIND rather than the code: a base class's +`@timing.clock` was dropped because `Design.initOwner` read only `__clsMeta.head`, and a base +TRAIT's was dropped a second time because the plugin gives traits no `__clsMeta` entry at all +(`transformTypeDef` skips them). Rewriting the reporter's `trait Bar` as `abstract class Bar` +took one probe and separated the two; patching only core then still left the reported case +failing, which reads like a wrong fix and is a second gate. Whenever a fact travels from the +plugin into elaboration, ask what the plugin CAPTURES and what core CONSUMES as two questions. + Probing designs outside the app runner has its own traps: a lib design class with all-defaulted parameters is auto-`@top`ed, and a bare `Design()` of a topped class returns a STAGED handle that never elaborates (no warnings, empty DB) — mark probe designs `@top(false)`, which needs @@ -1446,6 +1456,15 @@ so a print-spec test re-derives the connectivity analysis for free — a compile to cover the post-stage re-run. Two `ElaborationChecksSpec` tests were written the wrong way here before the rule was clear; do not copy them as a model. +**When a spec is the only thing failing, decide printer-vs-spec by the CONVENTION, not by which +side reads better.** A `SourceFile.path` carries the PLATFORM separator (the IP printers build it +with `Paths.get(...).toString`, `GowinDesigner`/`VivadoSim`/`NVC` with `separatorChar`) and the +emitters normalize to `/` where a path goes into a generated script (`forceWindowsToLinuxPath`, +the literal `source ips/X.tcl`). `VendorIPPrinterSpec` hard-coded `"ips/X.tcl"` and so passed only +where the separator is `/`, failing on Windows with a bare `None.get`. Find the other construction +sites of the value before touching the producer, and give the lookup a `fail` that prints the +candidates, so the next mismatch reports itself instead of throwing `None.get`. + **Do not copy the reporter's code into the repo.** Issue reports usually carry no license. Write a minimal design of your own that exercises the same path; if the shape is fully covered by stage specs, no `issues/iNNN.scala` file is needed at all. diff --git a/compiler/ir/src/main/scala/dfhdl/compiler/ir/Meta.scala b/compiler/ir/src/main/scala/dfhdl/compiler/ir/Meta.scala index 4e4ef1c4c..1516e4f41 100644 --- a/compiler/ir/src/main/scala/dfhdl/compiler/ir/Meta.scala +++ b/compiler/ir/src/main/scala/dfhdl/compiler/ir/Meta.scala @@ -68,3 +68,45 @@ object Meta: // Scala declaration behind it): name only, unknown position, root namespace def named(name: String, namespace: String = ""): Meta = Meta(Some(name), Position.unknown, None, Nil, namespace) + + /** Fold a DFHDL class-inheritance chain (the plugin-injected `__clsMeta`, most-derived first) + * into the leaf's meta: the leaf's name, position, doc and namespace, carrying the class + * annotations of the WHOLE chain. + * + * A design/interface is emitted FLAT, so a base class has no construct of its own to hold an + * annotation and its annotations must reach the leaf. They are merged rather than concatenated + * because every consumer reads them with `collectFirst` (the resolved clk/rst timing, + * `flattenMode`, the purity marking): two `@timing.clock`s in one list would mean the base's + * fields are silently dropped instead of inherited. Same-kind signal constraints therefore merge + * field by field with the more-derived class taking priority, so a base's `rate` survives a leaf + * that sets only `edge`; annotation kinds that do not compose keep their most-derived occurrence + * first, which is that same priority as `collectFirst` reads it. + */ + def foldClsChain(chain: List[Meta]): Option[Meta] = chain match + case Nil => None + case leaf :: Nil => Some(leaf) + case leaf :: _ => + // most-derived first, so an already-accumulated annotation always outranks the incoming one + val folded = chain.flatMap(_.annotations) + .foldLeft(List.empty[HWAnnotation]) { (acc, base) => + var merged = false + val updated = acc.map { + case derived if merged => derived + case derived: constraints.SigConstraint => + base match + case baseSig: constraints.SigConstraint => + baseSig.merge(derived, withPriority = true) match + case Some(mergedSig) => + merged = true + mergedSig + case None => derived + case _ => derived + case derived => + if (derived == base) merged = true + derived + } + if (merged) updated else acc :+ base + } + Some(leaf.setAnnotations(folded)) + end foldClsChain +end Meta diff --git a/compiler/stages/src/test/scala/StagesSpec/PrintCodeStringSpec.scala b/compiler/stages/src/test/scala/StagesSpec/PrintCodeStringSpec.scala index eb5573383..19e01bb43 100644 --- a/compiler/stages/src/test/scala/StagesSpec/PrintCodeStringSpec.scala +++ b/compiler/stages/src/test/scala/StagesSpec/PrintCodeStringSpec.scala @@ -4132,4 +4132,50 @@ class PrintCodeStringSpec extends StageSpec(stageCreatesUnrefAnons = true): |end Top |""".stripMargin ) + // A design is emitted FLAT: a base class and a mixed-in trait have no construct of their own in + // the generated HDL, so their class annotations fold into the leaf design's meta. A trait gets + // no `__clsMeta` entry at all, so its annotation rides on the entry of the class that + // introduces it, and the whole chain folds at elaboration. + test("Class annotations are inherited from a base class and a mixed-in trait"): + @hw.constraints.timing.clock(rate = 100.MHz) + trait ClkBase extends RTDesign + @hw.constraints.timing.reset(active = _.low) + abstract class RstBase extends ClkBase + class InheritedAnnots extends RstBase: + val x = Bit <> IN + val y = Bit <> OUT + y := x + assertCodeString( + new InheritedAnnots, + """|@timing.reset(active = _.low) + |@timing.clock(rate = 100.MHz) + |class InheritedAnnots extends RTDesign: + | val x = Bit <> IN + | val y = Bit <> OUT + | y := x + |end InheritedAnnots + |""".stripMargin + ) + // Same-kind constraints do not stack: they merge field by field with the most-derived class + // taking priority, so the base's `rate` survives a leaf that sets only the `edge` while the + // leaf's `portName` replaces the base's. Every consumer reads these with `collectFirst`, so a + // plain concatenation would silently drop the base's rate instead of inheriting it. + test("An inherited constraint merges field by field with the derived class taking priority"): + @hw.constraints.timing.clock(rate = 100.MHz, portName = "clk_base") + abstract class ClkRateBase extends RTDesign + @hw.constraints.timing.clock(edge = _.falling, portName = "clk_leaf") + class ClkEdgeLeaf extends ClkRateBase: + val x = Bit <> IN + val y = Bit <> OUT + y := x + assertCodeString( + new ClkEdgeLeaf, + """|@timing.clock(rate = 100.MHz, edge = _.falling, portName = "clk_leaf") + |class ClkEdgeLeaf extends RTDesign: + | val x = Bit <> IN + | val y = Bit <> OUT + | y := x + |end ClkEdgeLeaf + |""".stripMargin + ) end PrintCodeStringSpec diff --git a/core/src/main/scala/dfhdl/core/Design.scala b/core/src/main/scala/dfhdl/core/Design.scala index 86d36273e..0e68ec20e 100644 --- a/core/src/main/scala/dfhdl/core/Design.scala +++ b/core/src/main/scala/dfhdl/core/Design.scala @@ -21,7 +21,8 @@ trait Design extends Container, HasClsMeta, HasClsArgs: getSet.setGlobalTag(ir.DFHDLVersionTag(dfhdl.dfhdlVersion)) // Build the design block directly from the `__clsMeta` chain (the // plugin-injected, per-class metadata, most-derived first). The leaf names - // the design (meta); for a blackbox IP, the base-most concrete class + // the design and the chain's class annotations fold into its meta + // (`Meta.foldClsChain`); for a blackbox IP, the base-most concrete class // extending the IP marker names the IP type (`typeName`). val chain = __clsMeta val instMode = mkInstMode match @@ -34,7 +35,7 @@ trait Design extends Container, HasClsMeta, HasClsArgs: if chain.nonEmpty && f.resourcePath.isEmpty => InstMode.BlackBox(f.copy(resourcePath = s"dfhdl-ips/${chain.last.name}")) case other => other - val blockDFC = chain.headOption match + val blockDFC = ir.Meta.foldClsChain(chain) match case Some(meta) => dfc.setMeta(meta) case None => dfc.anonymize Design.Block(__domainType, instMode)(using blockDFC) diff --git a/core/src/main/scala/dfhdl/core/HasClsMeta.scala b/core/src/main/scala/dfhdl/core/HasClsMeta.scala index bb3c4f9ab..3296a675a 100644 --- a/core/src/main/scala/dfhdl/core/HasClsMeta.scala +++ b/core/src/main/scala/dfhdl/core/HasClsMeta.scala @@ -10,7 +10,11 @@ trait HasClsMeta: // classes appear — abstract library bases are not processed). Containers build // their design block directly from this chain at creation, with no mutation: // the leaf (head) names the design/interface/resource, and for a blackbox IP - // the base-most class in the chain names the IP type. + // the base-most class in the chain names the IP type. A design/interface is + // emitted FLAT, so the whole chain's class annotations fold into the leaf's + // meta (`ir.Meta.foldClsChain`); a mixed-in TRAIT gets no entry of its own, so + // the plugin folds its annotations into the entry of the class that + // introduces it. protected def __clsMeta: List[ir.Meta] = Nil end HasClsMeta diff --git a/core/src/main/scala/dfhdl/core/Interface.scala b/core/src/main/scala/dfhdl/core/Interface.scala index 283c0cdb6..e719dbac3 100644 --- a/core/src/main/scala/dfhdl/core/Interface.scala +++ b/core/src/main/scala/dfhdl/core/Interface.scala @@ -29,8 +29,9 @@ abstract class Interface private[core] def mkInstMode: InstMode = InstMode.Interface private[dfhdl] def initOwner: TOwner = // Build the interface block directly from the `__clsMeta` chain (leaf - // names the interface). - val blockDFC = __clsMeta.headOption match + // names the interface, and the chain's class annotations fold into its + // meta the same way a design's do). + val blockDFC = ir.Meta.foldClsChain(__clsMeta) match case Some(meta) => dfc.setMeta(meta) case None => dfc.anonymize Design.Block(__domainType, mkInstMode)(using blockDFC) diff --git a/docs/user-guide/design-domains/index.md b/docs/user-guide/design-domains/index.md index 6106bce0a..a8de9af58 100644 --- a/docs/user-guide/design-domains/index.md +++ b/docs/user-guide/design-domains/index.md @@ -67,6 +67,22 @@ and inherits the rest from the elaboration defaults. The empty form `@timing.clo `@timing.reset()` forces the slot to appear (e.g. on a combinational or blackbox owner) while still deriving every field from the defaults. +Annotations are also inherited along the class hierarchy: a design generated from a class +carries the annotations of its base classes and mixed-in traits, so a shared clocking policy +can live on a base design that several designs extend. Where the same annotation appears more +than once, it merges field by field with the most derived class winning, exactly as a partial +annotation merges over the elaboration defaults. + +```scala +@timing.clock(rate = 100.MHz) +trait Board100MHz extends RTDesign + +// clocked at 100.MHz on the falling edge +@timing.clock(edge = _.falling) +class MyDesign extends Board100MHz: + ... +``` + #### Inclusion Policies - `AsNeeded`: Only emits clock/reset ports when actually used. - `AlwaysAtTop`: Always emits the ports at the top level (silenced with `@unused` if unused). diff --git a/lib/src/main/scala/dfhdl/tools/toolsCore/Vivado.scala b/lib/src/main/scala/dfhdl/tools/toolsCore/Vivado.scala index ff058561d..b249b34e9 100644 --- a/lib/src/main/scala/dfhdl/tools/toolsCore/Vivado.scala +++ b/lib/src/main/scala/dfhdl/tools/toolsCore/Vivado.scala @@ -22,7 +22,9 @@ object Vivado extends Builder, Programmer: override protected def windowsBinExec: String = "vivado.bat" protected def versionCmd: String = s"-version" protected def extractVersion(cmdRetStr: String): Option[String] = - val versionPattern = """vivado\s+v(\d+\.\d+)""".r + // the version banner casing differs between releases ("Vivado v2023.1" in 2023.1, + // "vivado v2024.1" in 2024.1), so the tool name is matched case-insensitively + val versionPattern = """(?i)vivado\s+v(\d+\.\d+)""".r versionPattern.findFirstMatchIn(cmdRetStr).map(_.group(1)) override protected[dfhdl] def buildPreprocess(cd: CompiledDesign)(using @@ -43,8 +45,10 @@ object Vivado extends Builder, Programmer: cd: CompiledDesign )(using CompilerOptions, BuilderOptions): CompiledDesign = given MemberGetSet = cd.stagedDB.getSet + // `-source` is accepted by every Vivado release, whereas `-script` is rejected by older ones + // (e.g., 2023.1). Both source the script and exit with the same code. exec( - s"-mode batch -script ${topName}.tcl" + s"-mode batch -source ${topName}.tcl" ) cd override protected[dfhdl] def producedFiles(using @@ -70,7 +74,7 @@ object Vivado extends Builder, Programmer: )(using CompilerOptions, ProgrammerOptions): CompiledDesign = given MemberGetSet = cd.stagedDB.getSet exec( - s"-mode batch -script ${topName}_prog.tcl" + s"-mode batch -source ${topName}_prog.tcl" ) cd end Vivado diff --git a/lib/src/main/scala/dfhdl/tools/toolsCore/VivadoSim.scala b/lib/src/main/scala/dfhdl/tools/toolsCore/VivadoSim.scala index 941c696af..6538c5910 100644 --- a/lib/src/main/scala/dfhdl/tools/toolsCore/VivadoSim.scala +++ b/lib/src/main/scala/dfhdl/tools/toolsCore/VivadoSim.scala @@ -20,7 +20,8 @@ trait VivadoSimCommon extends Linter, Simulator: final protected def versionCmd: String = "-version" final override protected def windowsBinExec: String = s"$binExec.bat" final protected def extractVersion(cmdRetStr: String): Option[String] = - val versionPattern = """Vivado Simulator\s+v(\d+\.\d+)""".r + // matched case-insensitively, since the vendor's version banner casing varies across releases + val versionPattern = """(?i)Vivado Simulator\s+v(\d+\.\d+)""".r versionPattern.findFirstMatchIn(cmdRetStr).map(_.group(1)) protected def suppressLine(line: String): Boolean = line.startsWith("INFO:") diff --git a/lib/src/test/scala/VendorIPPrinterSpec.scala b/lib/src/test/scala/VendorIPPrinterSpec.scala index 9888229a4..add663a50 100644 --- a/lib/src/test/scala/VendorIPPrinterSpec.scala +++ b/lib/src/test/scala/VendorIPPrinterSpec.scala @@ -1,14 +1,26 @@ package dfhdl import munit.* import dfhdl.hw.annotation.top -import dfhdl.compiler.ir.MemberGetSet +import dfhdl.compiler.ir.{MemberGetSet, SourceFile} import dfhdl.tools.toolsCore.{VivadoIPPrinter, QuartusPrimeIPPrinter} +import java.nio.file.Paths // The vendor IP generation scripts (Vivado `create_ip` tcl, Quartus qsys tcl) are emitted from the // IP block's design parameters. An IP block is a sub-design, so the values it was instantiated // with live at the instantiation site and must be resolved through it; asking the parameter // under the default cache policy leaves it opaque and used to crash the printer (`None.get`). class VendorIPPrinterSpec extends FunSuite: + // A `SourceFile.path` is a filesystem path relative to the commit folder, so it carries the + // PLATFORM separator (the IP printers build it with `Paths.get("ips").resolve(...)`, other + // tools with `separatorChar`); the scripts that reference these files normalize to `/` + // themselves at the point of emission. The expected path must therefore be built the same way: + // a literal "ips/..." only matches where the separator happens to be `/`. + private def ipScript(srcFiles: List[SourceFile], ipName: String): String = + val path = Paths.get("ips").resolve(s"$ipName.tcl").toString + srcFiles.find(_.path == path).map(_.contents).getOrElse( + fail(s"no `$path` among: ${srcFiles.map(_.path).mkString(", ")}") + ) + class VivadoCounter( val WIDTH: Int <> CONST = 8, val CLK_PORT: String <> CONST = "clk", @@ -40,8 +52,7 @@ class VendorIPPrinterSpec extends FunSuite: test("vendor IP scripts carry the applied parameter values"): val cd = Top().compile given MemberGetSet = cd.stagedDB.getSet - val vivadoTcl = new VivadoIPPrinter().getSourceFiles - .find(_.path == "ips/VivadoCounter.tcl").get.contents + val vivadoTcl = ipScript(new VivadoIPPrinter().getSourceFiles, "VivadoCounter") assertNoDiff( vivadoTcl, """|create_ip -name VivadoCounter -module_name VivadoCounter @@ -52,8 +63,7 @@ class VendorIPPrinterSpec extends FunSuite: |] [get_ips VivadoCounter] |""".stripMargin ) - val qsysTcl = new QuartusPrimeIPPrinter().getSourceFiles - .find(_.path == "ips/QsysCounter.tcl").get.contents + val qsysTcl = ipScript(new QuartusPrimeIPPrinter().getSourceFiles, "QsysCounter") assert(qsysTcl.contains("QsysCounter 2.5"), qsysTcl) assert( qsysTcl.contains( diff --git a/plugin/src/main/scala/plugin/MetaContextPlacerPhase.scala b/plugin/src/main/scala/plugin/MetaContextPlacerPhase.scala index 157c7e134..ef6cffe91 100644 --- a/plugin/src/main/scala/plugin/MetaContextPlacerPhase.scala +++ b/plugin/src/main/scala/plugin/MetaContextPlacerPhase.scala @@ -224,6 +224,25 @@ class MetaContextPlacerPhase(setting: Setting) extends CapturePhase, IdentityDen DefDef(sym, chain) end clsScalaArgsOverrideDef + // The annotations this class contributes to its `__clsMeta` entry: its own, followed by those + // of the TRAITS it introduces into the linearization. A trait gets no entry of its own + // (`transformTypeDef` injects `__clsMeta` into classes only), so an annotation declared + // on one has to ride on the class that mixes it in; taking only the traits the SUPERCLASS does + // not already carry keeps each trait's contribution to exactly one entry of the chain. The order + // is the linearization's (most-derived first), which is the priority order `Meta.foldClsChain` + // resolves conflicting annotations by at elaboration. + private def clsChainAnnotations(clsSym: ClassSymbol)(using + Context + ): List[Annotations.Annotation] = + val superBases = + if (clsSym.superClass.exists) clsSym.superClass.asClass.baseClasses.toSet + else Set.empty[ClassSymbol] + clsSym.staticAnnotations ++ + clsSym.baseClasses.drop(1) + .filter(bc => bc.is(Trait) && !superBases.contains(bc)) + .flatMap(_.staticAnnotations) + end clsChainAnnotations + // Build the // override protected def __clsMeta: List[ir.Meta] = // r__For_Plugin.metaGen(...) :: super.__clsMeta @@ -255,7 +274,7 @@ class MetaContextPlacerPhase(setting: Setting) extends CapturePhase, IdentityDen mkOptionString(Some(clsSym.getFinalName())), tree.positionTree, mkOptionString(clsSym.docString), - mkList(clsSym.staticAnnotations.map(a => reownLocalDefs(dropProxies(a.tree), sym))), + mkList(clsChainAnnotations(clsSym).map(a => reownLocalDefs(dropProxies(a.tree), sym))), mkNamespace(clsSym) ) )