From 80b92c7dd3adcad8487a3f8224846e381219bfa6 Mon Sep 17 00:00:00 2001 From: Jacob Lifshay Date: Thu, 26 Mar 2026 18:21:14 -0700 Subject: [PATCH] change vcd output to have module contents under instance's name, more closely matching how it works in verilog --- crates/fayalite/src/sim/vcd.rs | 159 +++++++--- crates/fayalite/tests/sim/expected/mod1.vcd | 15 +- .../tests/sim/expected/ripple_counter.vcd | 276 +----------------- .../tests/sim/expected/sim_only_connects.vcd | 64 +--- 4 files changed, 120 insertions(+), 394 deletions(-) diff --git a/crates/fayalite/src/sim/vcd.rs b/crates/fayalite/src/sim/vcd.rs index d970eb0f..ad3e9746 100644 --- a/crates/fayalite/src/sim/vcd.rs +++ b/crates/fayalite/src/sim/vcd.rs @@ -26,6 +26,7 @@ use std::{ collections::BTreeMap, fmt::{self, Write as _}, io, mem, + num::NonZeroU64, }; #[derive(Default, Clone)] @@ -186,6 +187,26 @@ impl fmt::Debug for VcdWriterDecls { } } +/// pass in scope to ensure it's not available in child scope +fn try_write_vcd_scope( + writer: &mut W, + scope_type: &str, + scope_name: Interned, + scope: Option<&mut Scope>, + f: impl FnOnce(&mut W, Option<&mut Scope>) -> io::Result, +) -> io::Result { + let Some(scope) = scope else { + return f(writer, None); + }; + write_vcd_scope( + writer, + scope_type, + scope_name, + scope, + move |writer, scope| f(writer, Some(scope)), + ) +} + /// pass in scope to ensure it's not available in child scope fn write_vcd_scope( writer: &mut W, @@ -237,6 +258,7 @@ trait_arg! { struct ArgModule<'a> { properties: &'a mut VcdWriterProperties, scope: &'a mut Scope, + instance_name: Option>, } impl<'a> ArgModule<'a> { @@ -244,6 +266,7 @@ impl<'a> ArgModule<'a> { ArgModule { properties: self.properties, scope: self.scope, + instance_name: self.instance_name, } } } @@ -267,7 +290,7 @@ struct ArgInType<'a> { sink_var_type: &'static str, duplex_var_type: &'static str, properties: &'a mut VcdWriterProperties, - scope: &'a mut Scope, + scope: Option<&'a mut Scope>, } impl<'a> ArgInType<'a> { @@ -277,7 +300,7 @@ impl<'a> ArgInType<'a> { sink_var_type: self.sink_var_type, duplex_var_type: self.duplex_var_type, properties: self.properties, - scope: self.scope, + scope: self.scope.as_deref_mut(), } } } @@ -314,7 +337,7 @@ impl WriteTrace for TraceScalar { #[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)] #[repr(transparent)] -struct VcdId(u64); +struct VcdId(NonZeroU64); impl VcdId { const CHAR_RANGE: std::ops::RangeInclusive = b'!'..=b'~'; @@ -344,11 +367,14 @@ impl VcdId { }; retval = v; } + let Some(retval) = NonZeroU64::new(retval) else { + return None; + }; Some(Self(retval)) } #[must_use] const fn write(self, out: &mut [u8]) -> usize { - let mut id = self.0; + let mut id = self.0.get(); let mut len = 0; loop { let digit = (id % Self::BASE as u64) as u8 + *Self::CHAR_RANGE.start(); @@ -363,7 +389,7 @@ impl VcdId { } len } - const MAX_ID_LEN: usize = Self(u64::MAX).write(&mut []); + const MAX_ID_LEN: usize = Self(NonZeroU64::MAX).write(&mut []); } /// check that VcdId properly round-trips @@ -423,7 +449,7 @@ impl fmt::Display for Escaped { fn write_vcd_var( properties: &mut VcdWriterProperties, - scope: &mut Scope, + scope: Option<&mut Scope>, memory_element_part_body: MemoryElementPartBody, writer: &mut W, var_type: &str, @@ -431,8 +457,6 @@ fn write_vcd_var( location: TraceLocation, name: Interned, ) -> io::Result<()> { - let path_hash = scope.path_hash.clone().joined(name); - let name = scope.new_identifier(name); let id = match location { TraceLocation::Scalar(id) => id.as_usize(), TraceLocation::Memory(TraceMemoryLocation { @@ -464,12 +488,21 @@ fn write_vcd_var( first_id + *element_index } }; - let id = properties - .scalar_id_to_vcd_id_map - .builder_get_or_insert(id, &path_hash); - write!(writer, "$var {var_type} {size} ")?; - write_vcd_id(writer, id)?; - writeln!(writer, " {name} $end") + if let Some(scope) = scope { + let path_hash = scope.path_hash.clone().joined(name); + let name = scope.new_identifier(name); + let id = properties + .scalar_id_to_vcd_id_map + .builder_get_or_insert(id, &path_hash); + write!(writer, "$var {var_type} {size} ")?; + write_vcd_id(writer, id)?; + writeln!(writer, " {name} $end") + } else { + properties + .scalar_id_to_vcd_id_map + .builder_unused_scalar_id(id); + Ok(()) + } } impl WriteTrace for TraceUInt { @@ -712,14 +745,24 @@ impl WriteTrace for TraceScope { impl WriteTrace for TraceModule { fn write_trace(self, writer: &mut W, mut arg: A) -> io::Result<()> { - let ArgModule { properties, scope } = arg.module(); + let ArgModule { + properties, + scope, + instance_name, + } = arg.module(); let Self { name, children } = self; - write_vcd_scope(writer, "module", name, scope, |writer, scope| { - for child in children { - child.write_trace(writer, ArgModuleBody { properties, scope })?; - } - Ok(()) - }) + write_vcd_scope( + writer, + "module", + instance_name.unwrap_or(name), + scope, + |writer, scope| { + for child in children { + child.write_trace(writer, ArgModuleBody { properties, scope })?; + } + Ok(()) + }, + ) } } @@ -727,7 +770,7 @@ impl WriteTrace for TraceInstance { fn write_trace(self, writer: &mut W, mut arg: A) -> io::Result<()> { let ArgModuleBody { properties, scope } = arg.module_body(); let Self { - name: _, + name, instance_io, module, ty: _, @@ -739,10 +782,17 @@ impl WriteTrace for TraceInstance { sink_var_type: "wire", duplex_var_type: "wire", properties, - scope, + scope: None, }, )?; - module.write_trace(writer, ArgModule { properties, scope }) + module.write_trace( + writer, + ArgModule { + properties, + scope, + instance_name: Some(name), + }, + ) } } @@ -781,7 +831,7 @@ impl WriteTrace for TraceMem { sink_var_type: "reg", duplex_var_type: "reg", properties, - scope, + scope: Some(scope), }, ) }, @@ -813,7 +863,7 @@ impl WriteTrace for TraceMemPort { sink_var_type: "wire", duplex_var_type: "wire", properties, - scope, + scope: Some(scope), }, ) } @@ -834,7 +884,7 @@ impl WriteTrace for TraceWire { sink_var_type: "wire", duplex_var_type: "wire", properties, - scope, + scope: Some(scope), }, ) } @@ -855,7 +905,7 @@ impl WriteTrace for TraceReg { sink_var_type: "reg", duplex_var_type: "reg", properties, - scope, + scope: Some(scope), }, ) } @@ -877,7 +927,7 @@ impl WriteTrace for TraceModuleIO { sink_var_type: "wire", duplex_var_type: "wire", properties, - scope, + scope: Some(scope), }, ) } @@ -898,7 +948,7 @@ impl WriteTrace for TraceBundle { ty: _, flow: _, } = self; - write_vcd_scope(writer, "struct", name, scope, |writer, scope| { + try_write_vcd_scope(writer, "struct", name, scope, |writer, mut scope| { for field in fields { field.write_trace( writer, @@ -907,7 +957,7 @@ impl WriteTrace for TraceBundle { sink_var_type, duplex_var_type, properties, - scope, + scope: scope.as_deref_mut(), }, )?; } @@ -931,7 +981,7 @@ impl WriteTrace for TraceArray { ty: _, flow: _, } = self; - write_vcd_scope(writer, "struct", name, scope, |writer, scope| { + try_write_vcd_scope(writer, "struct", name, scope, |writer, mut scope| { for element in elements { element.write_trace( writer, @@ -940,7 +990,7 @@ impl WriteTrace for TraceArray { sink_var_type, duplex_var_type, properties, - scope, + scope: scope.as_deref_mut(), }, )?; } @@ -965,7 +1015,7 @@ impl WriteTrace for TraceEnumWithFields { ty: _, flow: _, } = self; - write_vcd_scope(writer, "struct", name, scope, |writer, scope| { + try_write_vcd_scope(writer, "struct", name, scope, |writer, mut scope| { discriminant.write_trace( writer, ArgInType { @@ -973,7 +1023,7 @@ impl WriteTrace for TraceEnumWithFields { sink_var_type, duplex_var_type, properties, - scope, + scope: scope.as_deref_mut(), }, )?; for field in non_empty_fields { @@ -984,7 +1034,7 @@ impl WriteTrace for TraceEnumWithFields { sink_var_type, duplex_var_type, properties, - scope, + scope: scope.as_deref_mut(), }, )?; } @@ -1026,6 +1076,7 @@ impl TraceWriterDecls for VcdWriterDecls { ArgModule { properties: &mut properties, scope: &mut Scope::new(PathHash::default()), + instance_name: None, }, )?; let ScalarIdToVcdIdMapOrBuilder::Builder(scalar_id_to_vcd_id_map_builder) = @@ -1065,23 +1116,29 @@ struct MemoryProperties { } struct ScalarIdToVcdIdMap { - scalar_id_to_vcd_id_map: Box<[VcdId]>, + scalar_id_to_vcd_id_map: Box<[Option]>, } #[derive(Default)] struct ScalarIdToVcdIdMapBuilder { - scalar_id_to_vcd_id_map: BTreeMap, + scalar_id_to_vcd_id_map: BTreeMap>, lower_half_to_next_upper_half_map: HashMap, } impl ScalarIdToVcdIdMapBuilder { + fn unused_scalar_id(&mut self, scalar_id: usize) { + self.scalar_id_to_vcd_id_map + .entry(scalar_id) + .or_insert(None); + } /// `VcdId`s are based off of `path_hash` (and not `scalar_id`) since the hash doesn't change /// when unrelated variables are added/removed, making the generated VCD more friendly for git diff. fn get_or_insert(&mut self, scalar_id: usize, path_hash: &PathHash) -> VcdId { *self .scalar_id_to_vcd_id_map .entry(scalar_id) - .or_insert_with(|| { + .or_insert(None) + .get_or_insert_with(|| { let hash = u128::from_le_bytes( *path_hash .0 @@ -1094,7 +1151,7 @@ impl ScalarIdToVcdIdMapBuilder { let next_upper_half = self .lower_half_to_next_upper_half_map .entry(lower_half) - .or_insert(0); + .or_insert(if lower_half == 0 { 1 } else { 0 }); let upper_half = *next_upper_half; *next_upper_half += 1; let Some(id) = upper_half @@ -1103,7 +1160,7 @@ impl ScalarIdToVcdIdMapBuilder { else { panic!("too many VcdIds"); }; - VcdId(id) + VcdId(NonZeroU64::new(id).expect("known to not be zero")) }) } fn build(self) -> ScalarIdToVcdIdMap { @@ -1129,7 +1186,7 @@ enum ScalarIdToVcdIdMapOrBuilder { } impl ScalarIdToVcdIdMapOrBuilder { - fn built_scalar_id_to_vcd_id(&self, scalar_id: usize) -> VcdId { + fn built_scalar_id_to_vcd_id(&self, scalar_id: usize) -> Option { let Self::Built(v) = self else { panic!("ScalarIdToVcdIdMap isn't built yet"); }; @@ -1141,6 +1198,12 @@ impl ScalarIdToVcdIdMapOrBuilder { }; v.get_or_insert(scalar_id, path_hash) } + fn builder_unused_scalar_id(&mut self, scalar_id: usize) { + let Self::Builder(v) = self else { + panic!("ScalarIdToVcdIdMap is already built"); + }; + v.unused_scalar_id(scalar_id) + } } struct VcdWriterProperties { @@ -1165,8 +1228,11 @@ impl VcdWriter { fn write_string_value_change( writer: &mut impl io::Write, value: impl fmt::Display, - id: VcdId, + id: Option, ) -> io::Result<()> { + let Some(id) = id else { + return Ok(()); + }; write!(writer, "s{} ", Escaped(value))?; write_vcd_id(writer, id)?; writer.write_all(b"\n") @@ -1175,8 +1241,11 @@ fn write_string_value_change( fn write_bits_value_change( writer: &mut impl io::Write, value: &BitSlice, - id: VcdId, + id: Option, ) -> io::Result<()> { + let Some(id) = id else { + return Ok(()); + }; match value.len() { 0 => writer.write_all(b"s0 ")?, 1 => writer.write_all(if value[0] { b"1" } else { b"0" })?, @@ -1205,7 +1274,7 @@ fn write_enum_discriminant_value_change( writer: &mut impl io::Write, variant_index: usize, ty: Enum, - id: VcdId, + id: Option, ) -> io::Result<()> { write_string_value_change( writer, diff --git a/crates/fayalite/tests/sim/expected/mod1.vcd b/crates/fayalite/tests/sim/expected/mod1.vcd index bd8676f3..b12db8fd 100644 --- a/crates/fayalite/tests/sim/expected/mod1.vcd +++ b/crates/fayalite/tests/sim/expected/mod1.vcd @@ -6,18 +6,12 @@ $var wire 2 Q2~aG o $end $var wire 2 DXK'| i2 $end $var wire 4 cPuix o2 $end $upscope $end -$scope struct child $end +$scope module child $end $var wire 4 ($5K7 i $end $var wire 2 %6Wv" o $end $var wire 2 +|-AU i2 $end $var wire 4 Hw?%j o2 $end $upscope $end -$scope module mod1_child $end -$var wire 4 4}s%= i $end -$var wire 2 }IY?g o $end -$var wire 2 of42K i2 $end -$var wire 4 D9]&= o2 $end -$upscope $end $upscope $end $enddefinitions $end $dumpvars @@ -25,10 +19,6 @@ b11 avK(^ b11 Q2~aG b10 DXK'| b1110 cPuix -b11 4}s%= -b11 }IY?g -b10 of42K -b1110 D9]&= b11 ($5K7 b11 %6Wv" b10 +|-AU @@ -38,9 +28,6 @@ $end b1010 avK(^ b10 Q2~aG b1111 cPuix -b1010 4}s%= -b10 }IY?g -b1111 D9]&= b1010 ($5K7 b10 %6Wv" b1111 Hw?%j diff --git a/crates/fayalite/tests/sim/expected/ripple_counter.vcd b/crates/fayalite/tests/sim/expected/ripple_counter.vcd index 80715507..b8002821 100644 --- a/crates/fayalite/tests/sim/expected/ripple_counter.vcd +++ b/crates/fayalite/tests/sim/expected/ripple_counter.vcd @@ -11,32 +11,20 @@ $var wire 1 d,)j~ \[4] $end $var wire 1 7?<~c \[5] $end $upscope $end $var reg 1 5Vse~ bit_reg_0 $end -$scope struct bit_reg_1 $end +$scope module bit_reg_1 $end $var wire 1 3GuTz clk $end $var wire 1 UHkx. o $end $upscope $end -$scope module sw_reg $end -$var wire 1 -3~F" clk $end -$var wire 1 `Z15M o $end -$upscope $end $var reg 1 .$$fF bit_reg_2 $end -$scope struct bit_reg_3 $end +$scope module bit_reg_3 $end $var wire 1 .Wa21 clk $end $var wire 1 _`|^x o $end $upscope $end -$scope module sw_reg_2 $end -$var wire 1 -3~F"" clk $end -$var wire 1 `Z15M" o $end -$upscope $end $var reg 1 eTSbN bit_reg_4 $end -$scope struct bit_reg_5 $end +$scope module bit_reg_5 $end $var wire 1 %U['n clk $end $var wire 1 ?Kpc. o $end $upscope $end -$scope module sw_reg_3 $end -$var wire 1 -3~F"# clk $end -$var wire 1 `Z15M# o $end -$upscope $end $upscope $end $enddefinitions $end $dumpvars @@ -49,18 +37,12 @@ b0 f0Cen 0d,)j~ 07?<~c 05Vse~ -0-3~F" -0`Z15M 03GuTz 0UHkx. 0.$$fF -0-3~F"" -0`Z15M" 0.Wa21 0_`|^x 0eTSbN -0-3~F"# -0`Z15M# 0%U['n 0?Kpc. $end @@ -69,27 +51,21 @@ $end b1 f0Cen 1-$(J- 15Vse~ -1-3~F" 13GuTz b111 f0Cen 1 out2 $end $var string 1 8(7-4 out3 $end -$scope struct helper1 $end +$scope module helper1 $end $scope struct cd $end $var wire 1 $Kwp\ clk $end $var wire 1 nmVq' rst $end @@ -16,17 +16,9 @@ $upscope $end $var string 1 qS)@z inp $end $var string 1 ~je// out $end $upscope $end -$scope module sim_only_connects_helper $end -$scope struct cd $end -$var wire 1 %uCn6 clk $end -$var wire 1 Apu`K rst $end -$upscope $end -$var string 1 $U*lA inp $end -$var string 1 !prwC out $end -$upscope $end $var string 1 CyjVm delay1 $end $var reg 1 z~g{\ delay1_empty $end -$scope struct helper2 $end +$scope module helper2 $end $scope struct cd $end $var wire 1 Ph.=# clk $end $var wire 1 !GXK\ rst $end @@ -34,14 +26,6 @@ $upscope $end $var string 1 /YVv: inp $end $var string 1 Kk*{# out $end $upscope $end -$scope module sim_only_connects_helper_2 $end -$scope struct cd $end -$var wire 1 %uCn6" clk $end -$var wire 1 Apu`K" rst $end -$upscope $end -$var string 1 $U*lA" inp $end -$var string 1 !prwC" out $end -$upscope $end $upscope $end $enddefinitions $end $dumpvars @@ -51,20 +35,12 @@ s{\"extra\":\x20\"value\"} g:xf? s{} [OKKg s{} 9pB-> s{} 8(7-4 -0%uCn6 -1Apu`K -s{} $U*lA -s{} !prwC 0$Kwp\ 1nmVq' s{} qS)@z s{} ~je// s{} CyjVm 0z~g{\ -0%uCn6" -1Apu`K" -s{} $U*lA" -s{} !prwC" 0Ph.=# 1!GXK\ s{} /YVv: @@ -73,110 +49,74 @@ $end #1000000 1tq:(w s{\"extra\":\x20\"value\"} [OKKg -1%uCn6 -s{\"extra\":\x20\"value\"} $U*lA 1$Kwp\ s{\"extra\":\x20\"value\"} qS)@z 1z~g{\ -1%uCn6" 1Ph.=# s{\"bar\":\x20\"\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} 9pB-> -s{\"bar\":\x20\"\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} !prwC s{\"bar\":\x20\"\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} ~je// -s{\"bar\":\x20\"\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} $U*lA" s{\"bar\":\x20\"\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} /YVv: s{\"bar\":\x20\"baz\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} 8(7-4 -s{\"bar\":\x20\"baz\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} !prwC" s{\"bar\":\x20\"baz\",\x20\"extra\":\x20\"value\",\x20\"foo\":\x20\"baz\"} Kk*{# #2000000 0tq:(w 0FVlgb -0%uCn6 -0Apu`K 0$Kwp\ 0nmVq' -0%uCn6" -0Apu`K" 0Ph.=# 0!GXK\ #3000000 1tq:(w -1%uCn6 1$Kwp\ s{\"extra\":\x20\"value\"} CyjVm 0z~g{\ -1%uCn6" 1Ph.=# #4000000 0tq:(w -0%uCn6 0$Kwp\ -0%uCn6" 0Ph.=# #5000000 1tq:(w -1%uCn6 1$Kwp\ -1%uCn6" 1Ph.=# #6000000 0tq:(w -0%uCn6 0$Kwp\ -0%uCn6" 0Ph.=# #7000000 1tq:(w -1%uCn6 1$Kwp\ -1%uCn6" 1Ph.=# #8000000 0tq:(w -0%uCn6 0$Kwp\ -0%uCn6" 0Ph.=# #9000000 1tq:(w -1%uCn6 1$Kwp\ -1%uCn6" 1Ph.=# #10000000 0tq:(w -0%uCn6 0$Kwp\ -0%uCn6" 0Ph.=# #11000000 1tq:(w -1%uCn6 1$Kwp\ -1%uCn6" 1Ph.=# #12000000 0tq:(w -0%uCn6 0$Kwp\ -0%uCn6" 0Ph.=# #13000000 1tq:(w -1%uCn6 1$Kwp\ -1%uCn6" 1Ph.=# #14000000 0tq:(w -0%uCn6 0$Kwp\ -0%uCn6" 0Ph.=# #15000000 1tq:(w -1%uCn6 1$Kwp\ -1%uCn6" 1Ph.=# #16000000