diff --git a/xlsynth-driver/README.md b/xlsynth-driver/README.md index 8b554db4..59ccd542 100644 --- a/xlsynth-driver/README.md +++ b/xlsynth-driver/README.md @@ -13,8 +13,10 @@ cargo run -p xlsynth-driver -- --toolchain=$HOME/xlsynth-toolchain.toml \ cargo run -p xlsynth-driver -- --toolchain=$HOME/xlsynth-toolchain.toml \ dslx2pipeline ../sample-usage/src/sample.x add1 \ --delay_model=asap7 --pipeline_stages=2 -cargo run -p xlsynth-driver -- dslx2sv-types ../tests/structure_zoo.x \ - --sv_enum_case_naming_policy=unqualified +cargo run -p xlsynth-driver -- dslx2sv-types \ + --dslx_input_file=../tests/structure_zoo.x \ + --sv_enum_case_naming_policy=unqualified \ + --sv_struct_field_ordering=reversed ``` For a full list of options, run `xlsynth-driver --help`. @@ -376,6 +378,10 @@ Required flags: - `--sv_enum_case_naming_policy ` – controls whether enum members are emitted as unqualified case names (e.g. `Read`) or prefixed with the enum name (e.g. `OpType_Read`). +Optional flags: + +- `--sv_struct_field_ordering ` – controls the packed bit layout of generated `typedef struct packed` declarations by emitting members in DSLX declaration order (`as_declared`, the default) or in reverse declaration order (`reversed`). + ### `dslx-show`: Show a DSLX symbol definition Resolves and prints a DSLX symbol definition (enums, structs, type aliases, constants, functions, quickchecks). diff --git a/xlsynth-driver/src/dslx2sv_types.rs b/xlsynth-driver/src/dslx2sv_types.rs index c6ebb43c..8c7e335e 100644 --- a/xlsynth-driver/src/dslx2sv_types.rs +++ b/xlsynth-driver/src/dslx2sv_types.rs @@ -2,30 +2,31 @@ //! Execution path for the `xlsynth-driver dslx2sv-types` subcommand. //! -//! This module parses the CLI-selected enum-case naming policy and routes it to -//! the shared `SvBridgeBuilder` so callers (for example Bazel rules) can choose -//! whether generated SV enum members are emitted as case-only symbols or -//! enum-qualified symbols. +//! This module parses the CLI-selected SV emission policies and routes them to +//! the shared `SvBridgeBuilder` so callers (for example Bazel rules) can +//! choose enum-member spelling and packed-struct bit layout. use clap::ArgMatches; use crate::common::get_dslx_paths; use crate::toolchain_config::ToolchainConfig; -use xlsynth::sv_bridge_builder::SvEnumCaseNamingPolicy; +use xlsynth::sv_bridge_builder::{SvEnumCaseNamingPolicy, SvStructFieldOrderingPolicy}; /// Converts DSLX type definitions to SV type declarations and writes the result /// to stdout. /// /// This function is intentionally thin: it wires file contents, import search -/// paths, and the caller-selected [`SvEnumCaseNamingPolicy`] into the shared -/// bridge implementation. Passing `Unqualified` for a module whose enums reuse -/// case names across types will surface as a generation-time collision error in -/// the builder. +/// paths, and the caller-selected SV emission policies into the shared bridge +/// implementation. Passing `Unqualified` for a module whose enums reuse case +/// names across types will surface as a generation-time collision error in the +/// builder. Choosing `Reversed` for `struct_field_ordering_policy` changes the +/// packed bit layout of emitted SV structs, not only their textual order. pub fn dslx2sv_types( input_file: &std::path::Path, dslx_stdlib_path: Option<&std::path::Path>, search_paths: &[&std::path::Path], enum_case_naming_policy: SvEnumCaseNamingPolicy, + struct_field_ordering_policy: SvStructFieldOrderingPolicy, ) { log::info!("dslx2sv_types"); let dslx = std::fs::read_to_string(input_file).unwrap(); @@ -36,8 +37,10 @@ pub fn dslx2sv_types( let mut import_data = xlsynth::dslx::ImportData::new(dslx_stdlib_path, additional_search_path_views); - let mut builder = - xlsynth::sv_bridge_builder::SvBridgeBuilder::with_enum_case_policy(enum_case_naming_policy); + let mut builder = xlsynth::sv_bridge_builder::SvBridgeBuilder::with_policies( + enum_case_naming_policy, + struct_field_ordering_policy, + ); xlsynth::dslx_bridge::convert_leaf_module(&mut import_data, &dslx, input_file, &mut builder) .unwrap(); let sv = builder.build(); @@ -47,11 +50,12 @@ pub fn dslx2sv_types( /// Handles the `dslx2sv-types` subcommand from the top-level Clap dispatch. /// /// The CLI definition in `main.rs` requires and validates -/// `--sv_enum_case_naming_policy` using Clap's `ValueEnum` support on -/// [`SvEnumCaseNamingPolicy`], so this function can retrieve the typed value +/// `--sv_enum_case_naming_policy` and `--sv_struct_field_ordering` using +/// Clap's `ValueEnum` support on [`SvEnumCaseNamingPolicy`] and +/// [`SvStructFieldOrderingPolicy`], so this function can retrieve typed values /// directly and then delegate to [`dslx2sv_types`]. Calling this with -/// `ArgMatches` from another subcommand would panic because the code -/// unwraps/`expect`s required `dslx2sv-types` arguments. +/// `ArgMatches` from another subcommand would panic because the code unwraps +/// required `dslx2sv-types` arguments. pub fn handle_dslx2sv_types(matches: &ArgMatches, config: &Option) { log::info!("handle_dslx2sv_types"); let input_file = matches.get_one::("dslx_input_file").unwrap(); @@ -61,6 +65,9 @@ pub fn handle_dslx2sv_types(matches: &ArgMatches, config: &Option("sv_enum_case_naming_policy") .expect("clap should require sv_enum_case_naming_policy"); + let struct_field_ordering_policy = *matches + .get_one::("sv_struct_field_ordering") + .expect("clap should default sv_struct_field_ordering"); let paths = get_dslx_paths(matches, config); let dslx_stdlib_path = paths.stdlib_path.as_ref().map(|p| p.as_path()); @@ -70,5 +77,6 @@ pub fn handle_dslx2sv_types(matches: &ArgMatches, config: &Option::new()), + ) + .arg( + Arg::new("sv_struct_field_ordering") + .long("sv_struct_field_ordering") + .value_name("POLICY") + .help( + "Packed-struct layout policy for generated SystemVerilog packed structs; member order controls the packed bit layout", + ) + .default_value("as_declared") + .action(ArgAction::Set) + .value_parser(clap::builder::EnumValueParser::< + SvStructFieldOrderingPolicy, + >::new()), ), ) .subcommand( diff --git a/xlsynth-driver/tests/invoke_test.rs b/xlsynth-driver/tests/invoke_test.rs index 1ae27dac..98fcd274 100644 --- a/xlsynth-driver/tests/invoke_test.rs +++ b/xlsynth-driver/tests/invoke_test.rs @@ -808,6 +808,94 @@ fn test_dslx2sv_types_subcommand_enum_qualified_policy() { ); } +// Verifies: dslx2sv-types keeps the default packed-struct layout when the new +// struct field ordering flag is omitted. +// Catches: Regressions where adding the optional struct-ordering flag silently +// changes the default packed-struct member layout. +#[test] +fn test_dslx2sv_types_subcommand_struct_ordering_defaults_to_as_declared() { + let dslx = r#" +struct MyStruct { + first: u8, + second: u16, +} +"#; + let temp_dir = tempfile::tempdir().unwrap(); + let dslx_path = temp_dir.path().join("my_module.x"); + std::fs::write(&dslx_path, dslx).unwrap(); + + let command_path = env!("CARGO_BIN_EXE_xlsynth-driver"); + let output = Command::new(command_path) + .arg("dslx2sv-types") + .arg("--dslx_input_file") + .arg(dslx_path.to_str().unwrap()) + .arg("--sv_enum_case_naming_policy") + .arg("unqualified") + .output() + .unwrap(); + + assert!( + output.status.success(), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + xlsynth_test_helpers::assert_valid_sv(&stdout); + assert_eq!( + stdout.trim(), + r#"typedef struct packed { + logic [7:0] first; + logic [15:0] second; +} my_struct_t;"# + ); +} + +// Verifies: dslx2sv-types reverses packed-struct member declarations, which +// changes the emitted packed layout, when the new policy is selected. +// Catches: Regressions where the CLI accepts the new policy but the builder +// still emits the default packed layout. +#[test] +fn test_dslx2sv_types_subcommand_struct_ordering_reversed() { + let dslx = r#" +struct MyStruct { + first: u8, + second: u16, +} +"#; + let temp_dir = tempfile::tempdir().unwrap(); + let dslx_path = temp_dir.path().join("my_module.x"); + std::fs::write(&dslx_path, dslx).unwrap(); + + let command_path = env!("CARGO_BIN_EXE_xlsynth-driver"); + let output = Command::new(command_path) + .arg("dslx2sv-types") + .arg("--dslx_input_file") + .arg(dslx_path.to_str().unwrap()) + .arg("--sv_enum_case_naming_policy") + .arg("unqualified") + .arg("--sv_struct_field_ordering") + .arg("reversed") + .output() + .unwrap(); + + assert!( + output.status.success(), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + xlsynth_test_helpers::assert_valid_sv(&stdout); + assert_eq!( + stdout.trim(), + r#"typedef struct packed { + logic [15:0] second; + logic [7:0] first; +} my_struct_t;"# + ); +} + // Negative test: omitting the required enum-case naming policy flag returns a // CLI error instead of silently choosing a default. #[test] @@ -836,6 +924,39 @@ fn test_dslx2sv_types_requires_sv_enum_case_naming_policy_flag() { assert!(stderr.contains("required")); } +// Negative test: invalid struct field ordering policy values are rejected by +// CLI validation with a helpful allowed-values error. +#[test] +fn test_dslx2sv_types_rejects_invalid_sv_struct_field_ordering_flag_value() { + let dslx = "struct MyStruct { first: u8, second: u16 }"; + let temp_dir = tempfile::tempdir().unwrap(); + let dslx_path = temp_dir.path().join("my_module.x"); + std::fs::write(&dslx_path, dslx).unwrap(); + + let command_path = env!("CARGO_BIN_EXE_xlsynth-driver"); + let output = Command::new(command_path) + .arg("dslx2sv-types") + .arg("--dslx_input_file") + .arg(dslx_path.to_str().unwrap()) + .arg("--sv_enum_case_naming_policy") + .arg("unqualified") + .arg("--sv_struct_field_ordering") + .arg("bad_policy") + .output() + .unwrap(); + + assert!( + !output.status.success(), + "stdout: {}\nstderr: {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("bad_policy")); + assert!(stderr.contains("as_declared")); + assert!(stderr.contains("reversed")); +} + // Negative test: invalid enum-case naming policy values are rejected by CLI // validation with a helpful allowed-values error. #[test] diff --git a/xlsynth/src/sv_bridge_builder.rs b/xlsynth/src/sv_bridge_builder.rs index d5766d36..72457a31 100644 --- a/xlsynth/src/sv_bridge_builder.rs +++ b/xlsynth/src/sv_bridge_builder.rs @@ -42,6 +42,25 @@ pub enum SvEnumCaseNamingPolicy { EnumQualified, } +/// Selects how DSLX struct members are ordered in emitted SV packed struct +/// declarations. +/// +/// In SystemVerilog `typedef struct packed`, member declaration order defines +/// the packed bit layout. Choosing [`SvStructFieldOrderingPolicy::Reversed`] +/// therefore changes the generated wire contract, not just the textual output +/// order. This does not change the DSLX-side type model or the enum naming +/// policy. +#[cfg_attr(feature = "clap", derive(clap::ValueEnum))] +#[cfg_attr(feature = "clap", value(rename_all = "snake_case"))] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum SvStructFieldOrderingPolicy { + /// Emit members in the same order they are declared in DSLX. + AsDeclared, + /// Emit members in the reverse of their DSLX declaration order, which also + /// reverses their packed-layout positions in the generated SV. + Reversed, +} + /// Accumulates SV type declarations for a DSLX module while enforcing a flat /// generated-name namespace. /// @@ -58,6 +77,8 @@ pub struct SvBridgeBuilder { defined: HashSet, /// Controls how enum member symbols are derived before collision checks. enum_case_naming_policy: SvEnumCaseNamingPolicy, + /// Controls the emitted order of packed struct members. + struct_field_ordering_policy: SvStructFieldOrderingPolicy, } fn camel_to_snake(name: &str) -> String { @@ -295,6 +316,19 @@ fn convert_extern_type( } impl SvBridgeBuilder { + /// Creates a builder with explicit enum and struct emission policies. + pub fn with_policies( + enum_case_naming_policy: SvEnumCaseNamingPolicy, + struct_field_ordering_policy: SvStructFieldOrderingPolicy, + ) -> Self { + Self { + lines: vec![], + defined: HashSet::new(), + enum_case_naming_policy, + struct_field_ordering_policy, + } + } + /// Creates a builder with an explicit policy for enum member symbol /// naming. /// @@ -305,11 +339,10 @@ impl SvBridgeBuilder { /// case with the containing enum name to avoid cross-enum collisions in /// the flat generated SV namespace. pub fn with_enum_case_policy(enum_case_naming_policy: SvEnumCaseNamingPolicy) -> Self { - Self { - lines: vec![], - defined: HashSet::new(), + Self::with_policies( enum_case_naming_policy, - } + SvStructFieldOrderingPolicy::AsDeclared, + ) } /// Returns the generated SV source accumulated so far. @@ -359,6 +392,36 @@ impl SvBridgeBuilder { ), } } + + fn struct_member_line(member: &StructMemberData) -> Result { + let member_name = &member.name; + + // Note: this is the type that type inference determined the member is; i.e. it + // will be something like `BitsType`, `StructType`, `ArrayType`, + // etc. + let member_concrete_ty = &member.concrete_type; + + let member_annotated_ty = &member.type_annotation; + + if let Some(extern_ref) = get_extern_type_ref(member_annotated_ty, member_concrete_ty) { + Ok(format!(" {extern_ref} {member_name};")) + } else if let Some(type_ref_type_annotation) = + member_annotated_ty.to_type_ref_type_annotation() + { + let type_ref = type_ref_type_annotation.get_type_ref(); + let type_def = type_ref.get_type_definition(); + if let Some(type_alias) = type_def.to_type_alias() { + let sv_type_name = struct_name_to_sv(&type_alias.get_identifier()); + Ok(format!(" {sv_type_name} {member_name};")) + } else { + let member_sv_ty = convert_type(member_concrete_ty, None)?; + Ok(format!(" {member_sv_ty} {member_name};")) + } + } else { + let member_sv_ty = convert_type(member_concrete_ty, None)?; + Ok(format!(" {member_sv_ty} {member_name};")) + } + } } impl BridgeBuilder for SvBridgeBuilder { @@ -411,37 +474,21 @@ impl BridgeBuilder for SvBridgeBuilder { ) -> Result<(), XlsynthError> { let mut lines = vec![]; lines.push("typedef struct packed {".to_string()); - for member in members { - let member_name = &member.name; - - // Note: this is the type that type inference determined the member is; i.e. it - // will be something like `BitsType`, `StructType`, `ArrayType`, - // etc. - let member_concrete_ty = &member.concrete_type; - - let member_annotated_ty = &member.type_annotation; - - if let Some(extern_ref) = get_extern_type_ref(member_annotated_ty, member_concrete_ty) { - lines.push(format!(" {extern_ref} {member_name};")); - continue; - } - - // If the member_annotated_ty is a local alias, we want to emit the type via - // its local identifier. - if let Some(type_ref_type_annotation) = - member_annotated_ty.to_type_ref_type_annotation() - { - let type_ref = type_ref_type_annotation.get_type_ref(); - let type_def = type_ref.get_type_definition(); - if let Some(type_alias) = type_def.to_type_alias() { - let sv_type_name = struct_name_to_sv(&type_alias.get_identifier()); - lines.push(format!(" {sv_type_name} {member_name};")); - continue; - } - } - - let member_sv_ty = convert_type(member_concrete_ty, None)?; - lines.push(format!(" {member_sv_ty} {member_name};")); + let member_lines = + if self.struct_field_ordering_policy == SvStructFieldOrderingPolicy::AsDeclared { + members + .iter() + .map(Self::struct_member_line) + .collect::, _>>()? + } else { + members + .iter() + .rev() + .map(Self::struct_member_line) + .collect::, _>>()? + }; + for member_line in member_lines { + lines.push(member_line); } lines.push(format!("}} {};\n", struct_name_to_sv(dslx_name))); self.lines.push(lines.join("\n")); @@ -514,10 +561,23 @@ mod tests { fn simple_convert_for_test_with_policy( dslx: &str, enum_case_naming_policy: SvEnumCaseNamingPolicy, + ) -> Result { + simple_convert_for_test_with_policies( + dslx, + enum_case_naming_policy, + SvStructFieldOrderingPolicy::AsDeclared, + ) + } + + fn simple_convert_for_test_with_policies( + dslx: &str, + enum_case_naming_policy: SvEnumCaseNamingPolicy, + struct_field_ordering_policy: SvStructFieldOrderingPolicy, ) -> Result { let mut import_data = dslx::ImportData::default(); let path = std::path::PathBuf::from_str("/memfile/my_module.x").unwrap(); - let mut builder = SvBridgeBuilder::with_enum_case_policy(enum_case_naming_policy); + let mut builder = + SvBridgeBuilder::with_policies(enum_case_naming_policy, struct_field_ordering_policy); convert_leaf_module(&mut import_data, dslx, &path, &mut builder)?; Ok(builder.build()) } @@ -610,6 +670,30 @@ mod tests { ); } + #[test] + fn test_convert_leaf_module_struct_def_reversed_field_order() { + let dslx = r#" + struct MyStruct { + byte_array: u8[10], + word_data: u16, + } + "#; + let sv = simple_convert_for_test_with_policies( + dslx, + SvEnumCaseNamingPolicy::Unqualified, + SvStructFieldOrderingPolicy::Reversed, + ) + .unwrap(); + assert_eq!( + sv, + r#"typedef struct packed { + logic [15:0] word_data; + logic [9:0] [7:0] byte_array; +} my_struct_t; +"# + ); + } + #[test] fn test_convert_leaf_module_type_alias_to_bits_type_only() { let dslx = "type MyType = u8;"; diff --git a/xlsynth/tests/reversed_struct_field_ordering.x b/xlsynth/tests/reversed_struct_field_ordering.x new file mode 100644 index 00000000..d21f712a --- /dev/null +++ b/xlsynth/tests/reversed_struct_field_ordering.x @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: Apache-2.0 + +struct MyStruct { + first: u8, + second: u16, +} diff --git a/xlsynth/tests/sv_bridge_test.rs b/xlsynth/tests/sv_bridge_test.rs index c9a0a016..ffdedb60 100644 --- a/xlsynth/tests/sv_bridge_test.rs +++ b/xlsynth/tests/sv_bridge_test.rs @@ -4,7 +4,7 @@ use pretty_assertions::assert_eq; use xlsynth::{ dslx, dslx_bridge::convert_imported_module, - sv_bridge_builder::{SvBridgeBuilder, SvEnumCaseNamingPolicy}, + sv_bridge_builder::{SvBridgeBuilder, SvEnumCaseNamingPolicy, SvStructFieldOrderingPolicy}, }; /// Tests that we can convert the whole "structure_zoo.x" file to SystemVerilog. @@ -66,3 +66,37 @@ fn test_sv_bridge_structure_zoo() { std::fs::read_to_string("tests/want_structure_zoo.golden.sv").unwrap(); assert_eq!(got_sv, structure_zoo_sv_golden); } + +/// Tests that reversed struct-field ordering changes the emitted packed-struct +/// layout by reversing member declaration order. +#[test] +fn test_sv_bridge_struct_field_ordering_reversed() { + let _ = env_logger::builder().is_test(true).try_init(); + let mut import_data = dslx::ImportData::default(); + let dslx_path = "tests/reversed_struct_field_ordering.x"; + let dslx_contents = std::fs::read_to_string(dslx_path).unwrap(); + let typechecked = dslx::parse_and_typecheck( + &dslx_contents, + dslx_path, + "reversed_struct_field_ordering", + &mut import_data, + ) + .unwrap(); + + let mut builder = SvBridgeBuilder::with_policies( + SvEnumCaseNamingPolicy::Unqualified, + SvStructFieldOrderingPolicy::Reversed, + ); + convert_imported_module(&typechecked, &mut builder).unwrap(); + let got_sv = builder.build(); + + xlsynth_test_helpers::assert_valid_sv(&got_sv); + assert_eq!( + got_sv, + r#"typedef struct packed { + logic [15:0] second; + logic [7:0] first; +} my_struct_t; +"# + ); +}