Fix crash when compiling models with BoundStep outputs #13
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eprecation The deprecated IndexSet::remove() method was replaced with swap_remove() in the small-signal network analysis. However, this exposed a latent order- dependency bug where the analysis results differed based on iteration order. Root Cause: The algorithm had a circular dependency where analyze_value() checks if values are in small_signal_vals, but membership in that set depends on the analysis results. With platform-specific hash ordering (ahash::RandomState), this caused reactive/resistive contribution counts to swap on Windows MSYS2. Solution: Implemented an order-independent fixed-point algorithm with four phases: 1. Speculatively add ALL candidate nodes to small_signal_vals 2. Evaluate all candidates against this consistent set state 3. Remove speculative nodes that weren't confirmed 4. Add confirmed flows and remove resolved candidates This ensures all candidates see the same set state during evaluation, making the analysis deterministic regardless of iteration order while still supporting circular dependencies (e.g., noise nodes). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The compiler was producing non-deterministic output due to use of ahash's RandomState, which uses random seeding for its hash function. This caused iteration order of hash-based collections to vary between runs, leading to different compilation results and sporadic CI failures. Root cause: GVN (Global Value Numbering) used ahash::RandomState for hashing expressions. Different hash values across runs caused different equivalence class assignments and leader selections, which cascaded: 1. GVN picks different instruction leaders non-deterministically 2. Values get replaced differently via replace_uses() 3. op_dependent_insts BitSet gets populated differently 4. determine_evaluation() makes different linearization decisions 5. Noise sources/implicit equations are created vs. linearized differently This manifested as entire noise sources or implicit equations appearing or disappearing between runs - not just ordering differences. Changes: - Replace ahash::RandomState with BuildHasherDefault<FxHasher> in GVN - Replace AHashMap/AHashSet with IndexMap/IndexSet (with FxHasher) or HashMap/HashSet (with FxHasher) throughout the codebase - Use FxHasher consistently for deterministic hashing - Remove unused ahash dependencies from several crates Affected crates: mir_opt, mir_autodiff, sim_back, osdi, hir_def, hir_lower, basedb, mir, vfs, typed_indexmap, sourcegen 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
In one single commit to audit the differences better
allowing compilation on apple darwin on arm64
This error was discovered by checking the openvaf-r check, thank you.
This uses the newest sccache action see https://github.blog/changelog/2025-03-20-notification-of-upcoming-breaking-changes-in-github-actions/#decommissioned-cache-service-brownouts Signed-off-by: Kreijstal <kreijstal@hotmail.com>
TODO: switch branches, implicit equations.
This commit fixes multiple issues related to building and linking with LLVM 18 on macOS ARM64: 1. Use LLVM 18's clang for bitcode generation (build.rs) - Bitcode generated by Apple's clang is incompatible with LLVM 18 - Now uses clang from LLVM_SYS_181_PREFIX when available 2. Configure ld64.lld linker with proper flags (linker/src/lib.rs) - Detect and use ld64.lld from LLVM 18 when available - Automatically add -syslibroot flag using xcrun --show-sdk-path 3. Simplify macOS target configuration (aarch64_apple_darwin.rs) - Removed -lSystem from post_link_args - With -undefined dynamic_lookup, symbols are resolved at runtime - Added -platform_version flags required by ld64.lld 4. Update README with macOS build instructions - Add macOS dependency setup section with Homebrew instructions - Document build-macos.sh and test-macos.sh convenience scripts - Add section explaining integration tests - Clarify how to enable and run integration tests These changes ensure the compiler works correctly when built against LLVM 18, fixing "Unsupported stack probing method" and "library not found for -lSystem" errors. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates: - Configure macOS CI to use llvm@18 instead of latest llvm - Add xtask for setup on Mac. Unfortunatly we can't fix for straight cargo build/test without patching llvm-sys 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This change makes integration test snapshots platform-agnostic, allowing cross-platform CI testing without spurious failures due to different struct layouts on ARM64 vs x86-64. Removed values: - residual offset numbers (4 values per node) - react_ptr offset values in jacobian entries - instance_size and model_size values These values are calculated by LLVM using platform-specific ABI alignment rules (LLVMABISizeOfType and LLVMOffsetOfElement), causing differences between architectures. All semantic information (parameter names, types, node names, jacobian flags, etc.) is preserved. All integration test snapshots have been regenerated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
LLVM 18's ld64.lld requires -platform_version flags on both x86_64 and ARM64. Also fixes CI to use correct GitHub runners (macos-14 for ARM64, macos-13 for x86_64). Changes: - Add -platform_version flags to x86_64_apple_darwin target spec - Update CI matrix to use macos-14 for ARM64 builds (macos-13 is x86_64 only) This fixes the linker error: ld64.lld: error: must specify -platform_version ld64.lld: error: missing or unsupported -arch x86_64 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Root cause: snprintf was declared with ALL parameters as variadic `(...)` instead of having its first 3 parameters fixed: `(ptr, i64, ptr, ...)`. On ARM64, fixed and variadic parameters use different calling conventions: - Fixed parameters: passed in x0-x7 registers - Variadic parameters: special handling, different register/stack allocation When snprintf was declared as `i32 (...)`, LLVM generated code expecting all arguments to use the variadic calling convention. This caused: 1. Arguments passed in wrong registers/memory locations 2. Double values (like 0x3ff0000000000000 = 1.0) interpreted as pointers 3. SIGSEGV when snprintf tried to strlen() these bogus pointer values Fix: Changed intrinsics.rs line 100 to pass `args` instead of `&[]` to `ty_variadic_func()`. Now snprintf is correctly declared as: declare i32 @snprintf(ptr, i64, ptr, ...) This makes the first 3 parameters use the standard calling convention while only format arguments use varargs convention. Also removed duplicate parameter addition code in compilation_unit.rs (lines 452-455) which was likely a failed attempt to work around the incorrect function signature. Fixes: Integration test crashes on ARM64 macOS with LLVM 18 Affects: BSIM3, BSIM4, and other models with large instance structs Testing: All 28 integration tests now pass on ARM64 macOS 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Better CI
This commit resolves mostcompiler warnings across the codebase: **Lifetime syntax warnings:** - Fixed elided lifetime annotations in return types across mir, mir_build, mir_autodiff, mir_opt, verilogae, and melange crates - Added explicit '_ or 'a lifetime parameters where the compiler couldn't infer them correctly **Deprecated features:** - Replaced deprecated `std::intrinsics::transmute` with `std::mem::transmute` in basedb, hir, and mir_interpret crates - Removed deprecated `cargo-clippy` feature checks in bforest, replacing with direct clippy lint attributes **Unused code:** - Prefixed unused variable `b` with underscore in hir_def - Removed unused imports in hir_lower and mir_llvm - Added `#[allow(dead_code)]` to VoidAbortCallback struct in osdi **Unexpected cfg conditions:** - Declared `cross_compile` cfg in sourcegen/Cargo.toml - Declared `nightly` feature in openvaf-driver/Cargo.toml - Removed Py_3_8 conditionals as were using private libpython internals. TODO: add support for Python 13 METH_FASTCALL **Other:** - Removed unreachable pattern in osdi metadata matching - Added `#[allow(static_mut_refs)]` to PyInit_verilogae for Python C API compatibility The codebase now builds cleanly with one warning in release mode, related to the deprecation of from IndexSet::remove, which requires a more in-depth fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…eprecation The deprecated IndexSet::remove() method was replaced with swap_remove() in the small-signal network analysis. However, this exposed a latent order- dependency bug where the analysis results differed based on iteration order. Root Cause: The algorithm had a circular dependency where analyze_value() checks if values are in small_signal_vals, but membership in that set depends on the analysis results. With platform-specific hash ordering (ahash::RandomState), this caused reactive/resistive contribution counts to swap on Windows MSYS2. Solution: Implemented an order-independent fixed-point algorithm with four phases: 1. Speculatively add ALL candidate nodes to small_signal_vals 2. Evaluate all candidates against this consistent set state 3. Remove speculative nodes that weren't confirmed 4. Add confirmed flows and remove resolved candidates This ensures all candidates see the same set state during evaluation, making the analysis deterministic regardless of iteration order while still supporting circular dependencies (e.g., noise nodes). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…and 'claude-guidance' into merge-macos
Adds support for analog operators (limexp, ddt, idt, etc.) inside signal-dependent conditional statements via a new CLI flag. This behavior violates strict Verilog-A semantics but is required by some commercial foundry models (e.g., GF130 PDK diode models). Implementation: - Created CompilationOpts structure in hir crate for extensible options - Embedded CompilationOpts as field in top-level Opts structure to reduce parameter contagion (future options only require updating CompilationOpts) - Added --allow-analog-in-cond CLI flag with comprehensive help text - Threaded option through CompilationDB and validation logic - Modified BodyCtx::allow_analog_operator() to accept conditionals when enabled - Defaults to strict mode (false) in all contexts Testing: - UI test (ui/analog_in_cond.va) verifies errors in strict mode - UI test (ui_allow_analog_cond/diode_with_conditionals.va) validates flag-enabled compilation with realistic diode model - All existing tests updated to use CompilationOpts structure - Test infrastructure supports both strict and permissive modes Design rationale: CompilationOpts structure prevents code contagion - adding future compiler options only requires modifying the struct, not every Opts construction site or function signature throughout the codebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This adds parsing and HIR support for Verilog-A module instantiation syntax: module_name #(.param(value)) instance_name (port1, port2); This syntax is used in some foundry PDK models (e.g., GF130 ESD models) to instantiate primitive modules like resistors and capacitors with parameter overrides. Changes: - Add ModuleInst grammar to veriloga.ungram with ParamAssignments and PortConnections - Implement parser grammar with lookahead to distinguish module instantiation from net declarations - Add ModuleInstItem HIR type to store instance name, module type, parameter assignments, and port connections - Add HIR lowering for module instances - Module instances are recorded but don't participate in name resolution (they will be processed during elaboration/flattening) Note: This is parsing support only. The actual instantiation/flattening of modules is not implemented - the instances are simply stored in the HIR for potential future use. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The compute_outputs function was calling unwrap_unchecked() on PackedOption values without first checking if they were Some. When a PlaceKind::BoundStep output had a None value, this would return the reserved value (0x3FFFFFF) which caused an index out of bounds panic when inserted into the bitset. This fixes the crash when compiling the Skywater PDK ReRAM model which uses $bound_step(). Fixes OpenVAF/OpenVAF-Reloaded#10 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
robtaylor
force-pushed
the
allow-analog-in-conditionals
branch
11 times, most recently
from
ac1a82d to
e3617c8
Compare
robtaylor
force-pushed
the
allow-analog-in-conditionals
branch
2 times, most recently
from
eedcee9 to
f8505dd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a crash when compiling the Skywater PDK ReRAM model and other models that use
$bound_step().Problem
The
compute_outputsfunction insim_back/src/context.rswas callingunwrap_unchecked()onPackedOptionvalues without first checking if they wereSome. When aPlaceKind::BoundStepoutput had aNonevalue, this would return the reserved value (0x3FFFFFF) which caused an index out of bounds panic when inserted into the bitset.Stack trace:
Solution
Changed the code to use
if let Some(val) = val.expand()to properly check forNonevalues before inserting into the bitset, matching the pattern used in thecontributes == truebranch which usesfilter_map(PackedOption::expand).Testing
Fixes OpenVAF/OpenVAF-Reloaded#10
🤖 Generated with Claude Code