From f7cfcb6da1ea86bbde865c3f842abd0d4cebc839 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Thu, 10 Sep 2026 11:45:27 -0700 Subject: [PATCH 1/2] fix(sky130): preserve MOS parameters that Substrate does not model `convert_spice_mos` built a `MosParams` from `w`, `l` and `nf * m * mult` and dropped every other parameter, so importing a SPICE netlist through the `Sky130` schema silently lost information: * Junction areas and perimeters (`ad`, `as`, `pd`, `ps`) were discarded. The SKY130 models default these to zero, so a round trip removed all source/drain junction capacitance. For the foundry SRAM bitcell this cut the capacitance a 64-row column presents to a bit line from 7.30 fF to 2.83 fF, making read timing optimistic by 60%. * Stress parameters (`sa`, `sb`, `sd`) and resistances (`nrd`, `nrs`) were discarded. * `m` and `mult` were folded into `nf`. The models treat all three differently -- `nf` is a finger count that divides the supplied width, `m` is an instance multiplier, and `mult` only appears in the mismatch-slope expressions -- so collapsing them changes the device. `Primitive::Mos` now carries an `extra` map of the parameters Substrate does not model, populated on import and merged back in on export. `nf` is taken from the `nf` parameter alone. Where a schema derives a key that can also appear in `extra` -- `mult` for SRC NDA, `m` and `mult` for CDS -- the two are multiplied, since both are device multipliers. MOSFETs that Substrate generates itself carry an empty `extra` and are unaffected. Co-Authored-By: Claude Opus 5 (1M context) --- pdks/sky130/src/lib.rs | 244 ++++++++++++++++++++++----------------- pdks/sky130/src/mos.rs | 1 + pdks/sky130/src/tests.rs | 117 ++++++++++++++++++- 3 files changed, 257 insertions(+), 105 deletions(-) diff --git a/pdks/sky130/src/lib.rs b/pdks/sky130/src/lib.rs index 00f3ce12e..d018d4de5 100644 --- a/pdks/sky130/src/lib.rs +++ b/pdks/sky130/src/lib.rs @@ -113,6 +113,13 @@ pub enum Primitive { kind: MosKind, /// The MOSFET parameters. params: MosParams, + /// Device parameters that Substrate does not model, preserved verbatim. + /// + /// Populated when a MOSFET is imported from a SPICE netlist that sets parameters + /// beyond `w`, `l` and `nf` (junction areas and perimeters, stress parameters, + /// multiplicity, ...). Keys are lowercased, since SPICE parameter names are + /// case insensitive. Empty for MOSFETs that Substrate generates itself. + extra: HashMap, }, /// A precision resistor. PrecisionResistor(PrecisionResistor), @@ -135,6 +142,10 @@ impl scir::schema::Schema for Sky130 { type Primitive = Primitive; } +/// Parameters that [`MosParams`] models, and that therefore must not be duplicated in +/// [`Primitive::Mos::extra`]. +const MOS_SIZING_PARAMS: [&str; 3] = ["w", "l", "nf"]; + fn convert_spice_mos( kind: &str, params: &HashMap, ParamValue>, @@ -145,55 +156,58 @@ fn convert_spice_mos( Sky130Schema::Open | Sky130Schema::SrcNda => dec!(1e3), Sky130Schema::Cds => dec!(1e9), }; - let mult = i64::try_from( + let numeric = |name: &'static str| { params - .get(&UniCase::new(arcstr::literal!("mult"))) + .get(&UniCase::new(ArcStr::from(name))) .and_then(|expr| expr.get_numeric()) .copied() - .unwrap_or(dec!(1)), - ) - .map_err(|_| ConvError::InvalidParameter)?; - let m = i64::try_from( - params - .get(&UniCase::new(arcstr::literal!("m"))) - .and_then(|expr| expr.get_numeric()) - .copied() - .unwrap_or(dec!(1)), - ) - .map_err(|_| ConvError::InvalidParameter)?; + }; + // `m` and `mult` are deliberately not folded into `nf`: the SKY130 models treat all + // three differently, so collapsing them changes the device. They are preserved in + // `extra` instead, which round-trips them back into the exported netlist. + let extra = params + .iter() + .filter(|(k, _)| !MOS_SIZING_PARAMS.contains(&k.to_lowercase().as_str())) + .map(|(k, v)| (ArcStr::from(k.to_lowercase()), v.clone())) + .collect(); Ok(Primitive::Mos { kind, params: MosParams { - w: i64::try_from( - *params - .get(&UniCase::new(arcstr::literal!("w"))) - .and_then(|expr| expr.get_numeric()) - .ok_or(ConvError::MissingParameter)? - * scale, - ) - .map_err(|_| ConvError::InvalidParameter)?, - l: i64::try_from( - *params - .get(&UniCase::new(arcstr::literal!("l"))) - .and_then(|expr| expr.get_numeric()) - .ok_or(ConvError::MissingParameter)? - * scale, - ) - .map_err(|_| ConvError::InvalidParameter)?, - nf: i64::try_from( - params - .get(&UniCase::new(arcstr::literal!("nf"))) - .and_then(|expr| expr.get_numeric()) - .copied() - .unwrap_or(dec!(1)), - ) - .map_err(|_| ConvError::InvalidParameter)? - * m - * mult, + w: i64::try_from(numeric("w").ok_or(ConvError::MissingParameter)? * scale) + .map_err(|_| ConvError::InvalidParameter)?, + l: i64::try_from(numeric("l").ok_or(ConvError::MissingParameter)? * scale) + .map_err(|_| ConvError::InvalidParameter)?, + nf: i64::try_from(numeric("nf").unwrap_or(dec!(1))) + .map_err(|_| ConvError::InvalidParameter)?, }, + extra, }) } +/// Combines the parameters Substrate derives from [`MosParams`] with those preserved +/// verbatim in [`Primitive::Mos::extra`]. +/// +/// A key present in both is multiplied rather than overwritten. Every key a schema derives +/// from [`MosParams`] that can also appear in `extra` (`mult` for the SRC NDA schema, `m` +/// and `mult` for the CDS schema) is a device multiplier, so the total is the product. +/// Parameters are returned sorted by name to keep netlists stable. +fn merge_mos_params( + sizing: impl IntoIterator, + extra: HashMap, +) -> Vec<(ArcStr, ParamValue)> { + let mut merged: HashMap = extra; + for (key, value) in sizing { + let combined = match (merged.get(&key).and_then(|v| v.get_numeric()), &value) { + (Some(existing), ParamValue::Numeric(derived)) => (existing * derived).into(), + _ => value, + }; + merged.insert(key, combined); + } + let mut merged: Vec<_> = merged.into_iter().collect(); + merged.sort_by(|(a, _), (b, _)| a.cmp(b)); + merged +} + impl FromSchema for Sky130 { type Error = ConvError; @@ -310,23 +324,24 @@ impl FromSchema for Spice { .map(|(k, v)| (UniCase::new(k), v)) .collect(), }, - Primitive::Mos { kind, params } => spice::Primitive::RawInstance { + Primitive::Mos { + kind, + params, + extra, + } => spice::Primitive::RawInstance { cell: kind.open_subckt(), ports: vec!["D".into(), "G".into(), "S".into(), "B".into()], - params: HashMap::from_iter([ - ( - UniCase::new(arcstr::literal!("w")), - Decimal::new(params.w, 3).into(), - ), - ( - UniCase::new(arcstr::literal!("l")), - Decimal::new(params.l, 3).into(), - ), - ( - UniCase::new(arcstr::literal!("nf")), - Decimal::from(params.nf).into(), - ), - ]), + params: merge_mos_params( + [ + (arcstr::literal!("w"), Decimal::new(params.w, 3).into()), + (arcstr::literal!("l"), Decimal::new(params.l, 3).into()), + (arcstr::literal!("nf"), Decimal::from(params.nf).into()), + ], + extra, + ) + .into_iter() + .map(|(k, v)| (UniCase::new(k), v)) + .collect(), }, _ => unimplemented!("unsupported primitive"), }) @@ -373,14 +388,21 @@ impl FromSchema for Spectre { ports, params: params.into_iter().collect(), }, - Primitive::Mos { kind, params } => spectre::Primitive::RawInstance { + Primitive::Mos { + kind, + params, + extra, + } => spectre::Primitive::RawInstance { cell: kind.open_subckt(), ports: vec!["D".into(), "G".into(), "S".into(), "B".into()], - params: vec![ - (arcstr::literal!("w"), Decimal::new(params.w, 3).into()), - (arcstr::literal!("l"), Decimal::new(params.l, 3).into()), - (arcstr::literal!("nf"), Decimal::from(params.nf).into()), - ], + params: merge_mos_params( + [ + (arcstr::literal!("w"), Decimal::new(params.w, 3).into()), + (arcstr::literal!("l"), Decimal::new(params.l, 3).into()), + (arcstr::literal!("nf"), Decimal::from(params.nf).into()), + ], + extra, + ), }, _ => unimplemented!("unsupported primitive"), }) @@ -453,23 +475,24 @@ impl FromSchema for Spice { .map(|(k, v)| (UniCase::new(k), v)) .collect(), }, - Primitive::Mos { kind, params } => spice::Primitive::Mos { + Primitive::Mos { + kind, + params, + extra, + } => spice::Primitive::Mos { model: kind.src_nda_subckt(), - params: HashMap::from_iter([ - ( - UniCase::new(arcstr::literal!("w")), - Decimal::new(params.w, 3).into(), - ), - ( - UniCase::new(arcstr::literal!("l")), - Decimal::new(params.l, 3).into(), - ), - ( + params: merge_mos_params( + [ + (arcstr::literal!("w"), Decimal::new(params.w, 3).into()), + (arcstr::literal!("l"), Decimal::new(params.l, 3).into()), // Calibre decks don't support nf, so assign mult=nf instead. - UniCase::new(arcstr::literal!("mult")), - Decimal::from(params.nf).into(), - ), - ]), + (arcstr::literal!("mult"), Decimal::from(params.nf).into()), + ], + extra, + ) + .into_iter() + .map(|(k, v)| (UniCase::new(k), v)) + .collect(), }, Primitive::PrecisionResistor(res) => spice::Primitive::Res2 { value: spice::ComponentValue::Model("mrp".into()), @@ -516,14 +539,21 @@ impl FromSchema for Spectre { ports, params: params.into_iter().collect(), }, - Primitive::Mos { kind, params } => spectre::Primitive::RawInstance { + Primitive::Mos { + kind, + params, + extra, + } => spectre::Primitive::RawInstance { cell: kind.src_nda_subckt(), ports: vec!["D".into(), "G".into(), "S".into(), "B".into()], - params: vec![ - (arcstr::literal!("w"), Decimal::new(params.w, 3).into()), - (arcstr::literal!("l"), Decimal::new(params.l, 3).into()), - (arcstr::literal!("mult"), Decimal::from(params.nf).into()), - ], + params: merge_mos_params( + [ + (arcstr::literal!("w"), Decimal::new(params.w, 3).into()), + (arcstr::literal!("l"), Decimal::new(params.l, 3).into()), + (arcstr::literal!("mult"), Decimal::from(params.nf).into()), + ], + extra, + ), }, Primitive::PrecisionResistor(res) => spectre::Primitive::RawInstance { cell: "mrp".into(), @@ -606,23 +636,24 @@ impl FromSchema for Spice { .map(|(k, v)| (UniCase::new(k), v)) .collect(), }, - Primitive::Mos { kind, params } => spice::Primitive::Mos { + Primitive::Mos { + kind, + params, + extra, + } => spice::Primitive::Mos { model: kind.cds_subckt(), - params: HashMap::from_iter([ - ( - UniCase::new(arcstr::literal!("w")), - Decimal::new(params.w, 9).into(), - ), - ( - UniCase::new(arcstr::literal!("l")), - Decimal::new(params.l, 9).into(), - ), - ( - UniCase::new(arcstr::literal!("m")), - Decimal::from(params.nf).into(), - ), - (UniCase::new(arcstr::literal!("mult")), dec!(1).into()), - ]), + params: merge_mos_params( + [ + (arcstr::literal!("w"), Decimal::new(params.w, 9).into()), + (arcstr::literal!("l"), Decimal::new(params.l, 9).into()), + (arcstr::literal!("m"), Decimal::from(params.nf).into()), + (arcstr::literal!("mult"), dec!(1).into()), + ], + extra, + ) + .into_iter() + .map(|(k, v)| (UniCase::new(k), v)) + .collect(), }, _ => unimplemented!("unsupported primitive"), }) @@ -650,14 +681,21 @@ impl FromSchema for Spectre { ports, params: params.into_iter().collect(), }, - Primitive::Mos { kind, params } => spectre::Primitive::RawInstance { + Primitive::Mos { + kind, + params, + extra, + } => spectre::Primitive::RawInstance { cell: kind.cds_subckt(), ports: vec!["D".into(), "G".into(), "S".into(), "B".into()], - params: vec![ - (arcstr::literal!("w"), Decimal::new(params.w, 9).into()), - (arcstr::literal!("l"), Decimal::new(params.l, 9).into()), - (arcstr::literal!("m"), Decimal::from(params.nf).into()), - ], + params: merge_mos_params( + [ + (arcstr::literal!("w"), Decimal::new(params.w, 9).into()), + (arcstr::literal!("l"), Decimal::new(params.l, 9).into()), + (arcstr::literal!("m"), Decimal::from(params.nf).into()), + ], + extra, + ), }, _ => unimplemented!("unsupported primitive"), }) diff --git a/pdks/sky130/src/mos.rs b/pdks/sky130/src/mos.rs index 6cae9c414..39c0833ad 100644 --- a/pdks/sky130/src/mos.rs +++ b/pdks/sky130/src/mos.rs @@ -161,6 +161,7 @@ macro_rules! define_mosfets { let mut prim = substrate::schematic::PrimitiveBinding::new(crate::Primitive::Mos { kind: MosKind::$typ, params: self.params.clone(), + extra: Default::default(), }); prim.connect("D", io.d); prim.connect("G", io.g); diff --git a/pdks/sky130/src/tests.rs b/pdks/sky130/src/tests.rs index 44b756eb9..9271b3466 100644 --- a/pdks/sky130/src/tests.rs +++ b/pdks/sky130/src/tests.rs @@ -15,10 +15,12 @@ use rust_decimal::Decimal; use rust_decimal_macros::dec; use scir::ParamValue; use spectre::Spectre; +use spice::Spice; use std::any::Any; use std::collections::HashMap; use std::marker::PhantomData; use std::path::PathBuf; +use substrate::arcstr::ArcStr; use substrate::block::Block; use substrate::context::Context; use substrate::schematic::schema::Schema; @@ -355,12 +357,123 @@ fn test_convert_spice_mos() { let kind = "nshort"; let prim = convert_spice_mos(kind, ¶ms).expect("failed to convert mos"); match prim { - Primitive::Mos { kind, params } => { + Primitive::Mos { + kind, + params, + extra, + } => { assert_eq!(kind, MosKind::Nfet01v8); - assert_eq!(params.nf, 24); assert_eq!(params.w, 2_000); assert_eq!(params.l, 150); + // `m` and `mult` are not folded into `nf`; the models treat all three + // differently, so they are preserved separately. + assert_eq!(params.nf, 4); + assert_eq!( + extra.get(&arcstr::literal!("mult")), + Some(&ParamValue::Numeric(dec!(2))) + ); + assert_eq!( + extra.get(&arcstr::literal!("m")), + Some(&ParamValue::Numeric(dec!(3))) + ); } _ => panic!("bad primitive"), } } + +/// Parameters SKY130 models but Substrate does not, such as the junction areas and +/// perimeters carried by the foundry SRAM bitcell netlists, must survive a round trip +/// through the [`Sky130`] schema. Losing them silently zeroes the junction capacitance. +#[test] +fn test_convert_spice_mos_preserves_unmodelled_params() { + let params = HashMap::from_iter([ + ( + UniCase::new(arcstr::literal!("w")), + ParamValue::Numeric(dec!(0.14)), + ), + ( + UniCase::new(arcstr::literal!("l")), + ParamValue::Numeric(dec!(0.15)), + ), + ( + UniCase::new(arcstr::literal!("ad")), + ParamValue::Numeric(dec!(0.04375)), + ), + ( + UniCase::new(arcstr::literal!("pd")), + ParamValue::Numeric(dec!(0.92)), + ), + ( + UniCase::new(arcstr::literal!("as")), + ParamValue::Numeric(dec!(0.0168)), + ), + ( + UniCase::new(arcstr::literal!("ps")), + ParamValue::Numeric(dec!(0.52)), + ), + ]); + let prim = convert_spice_mos("sky130_fd_pr__nfet_01v8", ¶ms).expect("failed to convert"); + let exported = >::convert_primitive(prim) + .expect("failed to convert back to SPICE"); + let spice::Primitive::RawInstance { + cell, params: out, .. + } = exported + else { + panic!("expected a raw subcircuit instance"); + }; + assert_eq!(cell, "sky130_fd_pr__nfet_01v8"); + for (key, value) in [ + ("w", dec!(0.14)), + ("l", dec!(0.15)), + ("nf", dec!(1)), + ("ad", dec!(0.04375)), + ("pd", dec!(0.92)), + ("as", dec!(0.0168)), + ("ps", dec!(0.52)), + ] { + let actual = out + .get(&UniCase::new(ArcStr::from(key))) + .unwrap_or_else(|| panic!("missing parameter `{key}`")) + .get_numeric() + .copied() + .unwrap_or_else(|| panic!("parameter `{key}` is not numeric")); + assert_eq!(actual, value, "parameter `{key}`"); + } + assert_eq!(out.len(), 7, "unexpected parameters: {out:?}"); +} + +/// The SRC NDA schema exports `nf` as `mult`, so an imported `mult` must compose with it +/// rather than being dropped or duplicated. +#[test] +fn test_src_nda_mos_export_composes_multiplicity() { + let params = HashMap::from_iter([ + ( + UniCase::new(arcstr::literal!("w")), + ParamValue::Numeric(dec!(1)), + ), + ( + UniCase::new(arcstr::literal!("l")), + ParamValue::Numeric(dec!(0.15)), + ), + ( + UniCase::new(arcstr::literal!("nf")), + ParamValue::Numeric(dec!(3)), + ), + ( + UniCase::new(arcstr::literal!("mult")), + ParamValue::Numeric(dec!(2)), + ), + ]); + let prim = convert_spice_mos("nshort", ¶ms).expect("failed to convert"); + let exported = >::convert_primitive(prim) + .expect("failed to convert back to SPICE"); + let spice::Primitive::Mos { params: out, .. } = exported else { + panic!("expected a MOSFET"); + }; + assert_eq!( + out.get(&UniCase::new(arcstr::literal!("mult"))) + .and_then(|v| v.get_numeric()) + .copied(), + Some(dec!(6)) + ); +} From fb50b384ee96c37e3942b368aa52d25297b98641 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Thu, 17 Sep 2026 14:14:09 -0700 Subject: [PATCH 2/2] fix(spectre): escape signal names in save and ic statements `save` and `ic` wrote the name of a `SimSignal::Raw` verbatim. Spectre only accepts `[`, `]` and `#` in an identifier when they are escaped, so saving a bus bit or a node imported from layout -- `din[0]`, or `...Xdff_6.a_1800_291#` -- made Spectre reject the netlist during circuit read-in rather than run. `Spectre::escape_path` escapes those characters while preserving what gives a path its meaning: `.` between hierarchy elements, `:` before a terminal, and the `*`/`?` wildcards that `save` accepts. It is applied through a new `SimSignal::to_netlist_string`, so only the two netlisting call sites change; result lookup still uses the unescaped name, which is what Spectre reports back. Co-Authored-By: Claude Opus 5 (1M context) --- tools/spectre/src/lib.rs | 76 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 74 insertions(+), 2 deletions(-) diff --git a/tools/spectre/src/lib.rs b/tools/spectre/src/lib.rs index e0e2797be..6183664bc 100644 --- a/tools/spectre/src/lib.rs +++ b/tools/spectre/src/lib.rs @@ -56,6 +56,34 @@ pub mod analysis; pub mod blocks; pub mod error; pub(crate) mod templates; +#[cfg(test)] +mod escape_tests { + use super::Spectre; + + #[test] + fn escape_path_preserves_structure_and_escapes_the_rest() { + // `.` and `:` give a path its structure and must survive. + assert_eq!( + Spectre::escape_path("Xdut.X0.Xbitcell_array.Xcell_0_0.Q"), + "Xdut.X0.Xbitcell_array.Xcell_0_0.Q" + ); + assert_eq!(Spectre::escape_path("Xdut.Xinst:p"), "Xdut.Xinst:p"); + // Bus indices and layout-extracted names are not bare identifiers. + assert_eq!(Spectre::escape_path("din[0]"), "din\\[0\\]"); + assert_eq!( + Spectre::escape_path("Xdut.X0.Xdff_6.a_1800_291#"), + "Xdut.X0.Xdff_6.a_1800_291\\#" + ); + // `save` accepts wildcards, so they must not be escaped either. + assert_eq!(Spectre::escape_path("wl_b[*]"), "wl_b\\[*\\]"); + // Escaping an already escaped path is a no-op. + assert_eq!( + Spectre::escape_path(&Spectre::escape_path("din[0]")), + Spectre::escape_path("din[0]") + ); + } +} + #[cfg(test)] mod tests; @@ -169,6 +197,23 @@ impl SimSignal { Self::from(path) } + /// Renders this signal for a `save` or `ic` statement. + /// + /// [`SimSignal::Raw`] is a caller-supplied path that has not been escaped; the other + /// variants are built from SCIR names that are escaped as they are resolved. Result + /// lookup continues to use the unescaped [`SimSignal::to_string`], since Spectre + /// reports the unescaped name. + pub(crate) fn to_netlist_string( + &self, + lib: &Library, + conv: &NetlistLibConversion, + ) -> ArcStr { + match self { + SimSignal::Raw(raw) => ArcStr::from(Spectre::escape_path(raw)), + _ => self.to_string(lib, conv), + } + } + pub(crate) fn to_string(&self, lib: &Library, conv: &NetlistLibConversion) -> ArcStr { match self { SimSignal::Raw(raw) => raw.clone(), @@ -605,13 +650,13 @@ impl Spectre { writeln!(w, "settemp1 options temp={}", temp)?; } for save in saves { - writeln!(w, "save {}", save.to_string(&ctx.lib.scir, &conv))?; + writeln!(w, "save {}", save.to_netlist_string(&ctx.lib.scir, &conv))?; } if let Some(save) = options.save { writeln!(w, "setsave1 options save={}", save)?; } for (k, v) in ics { - writeln!(w, "ic {}={}", k.to_string(&ctx.lib.scir, &conv), v)?; + writeln!(w, "ic {}={}", k.to_netlist_string(&ctx.lib.scir, &conv), v)?; } writeln!(w)?; @@ -689,6 +734,33 @@ impl Spectre { escaped_name } + /// Escapes a hierarchical signal path for use in `save` and `ic` statements. + /// + /// Unlike [`Spectre::escape_identifier`], this preserves the characters that give a + /// path its structure: `.` separates hierarchy and `:` selects a terminal. Everything + /// else that cannot appear in a bare identifier -- `[` and `]` around bus indices, `#` + /// in names extracted from layout, and so on -- is backslash escaped, which is what + /// Spectre requires. The `*` and `?` wildcards accepted by `save` are also preserved. A character already preceded by a backslash is left alone so that + /// escaping an escaped path is a no-op. + pub fn escape_path(path: &str) -> String { + let mut escaped = String::with_capacity(path.len()); + let mut chars = path.chars(); + while let Some(c) = chars.next() { + if c == '\\' { + escaped.push(c); + if let Some(next) = chars.next() { + escaped.push(next); + } + } else if c.is_alphanumeric() || matches!(c, '_' | '.' | ':' | '*' | '?') { + escaped.push(c); + } else { + escaped.push('\\'); + escaped.push(c); + } + } + escaped + } + /// Converts a [`scir::InstancePath`] to a Spectre path string corresponding to /// the associated instance. pub fn instance_path(