Repository navigation
Array code after index reduction, component rolling, torn loops in the RHS (0.8.3) - #94
Conversation
…e RHS (0.8.3) - Models reduced in array form stay in array code: derivatives solved in array form, an OMJLIR front half (records, functions, if- and when-equations), slicing. - Component rolling on by default: separately declared components of one class as arrays. - Algebraic loops: Newton per element; large ones torn and solved in the RHS. - Simplification in array form: folding, aliases, semiLinear chains, inlining, invariants, crossings; initialization scaled, in a trust region. - Build: large generated functions split; plans kept across splits; a precompile workload for the arrays-kept paths. BACKEND_PERFLOG prints max RSS. - MSL EngineV6 in array code: faster than the scalar path, validates; MSL 4.1.0 coverage validate 503 (500). Assisted-by: Claude Opus 5.5
|
@CodeRabbit Review (Checking of it works) |
✅ Action performed
|
Walkthrough
Merge Risk:
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the primary changes: retaining array code after index reduction, component rolling, and RHS algebraic-loop handling. |
| Description check | |
The description is directly related to the changeset and provides detailed coverage of array code generation, component rolling, algebraic loops, events, performance, and tests. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (32 skipped: 32 unsupported.)
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
-
-
- Source mode: Auto
- Repository: Unavailable
- Branch: Unavailable
- File: No configuration file from this repository applied
- Commit: Unavailable
- Layers: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9ea58851-a689-461e-bcaa-dea0e1c5acb6
-
Project.tomlsrc/Backend/Passes/Evaluation/Elements.jlsrc/Backend/Passes/Evaluation/Evaluate.jlsrc/Backend/Passes/Evaluation/Interpret.jlsrc/Backend/Passes/IndexReduction/ArrayCommit.jlsrc/Backend/Passes/IndexReduction/ArraySlicing.jlsrc/Backend/Passes/IndexReduction/ArraySolve.jlsrc/Backend/Passes/IndexReduction/Arrays.jlsrc/Backend/Passes/IndexReduction/ComponentRolling.jlsrc/Backend/Passes/IndexReduction/Differentiate.jlsrc/Backend/Passes/IndexReduction/IndexReduction.jlsrc/Backend/Passes/IndexReduction/SolveDerivatives.jlsrc/Backend/Passes/IndexReduction/Structure.jlsrc/Backend/Passes/causalize.jlsrc/Backend/Passes/util.jlsrc/CodeGeneration/ArrayBlockSolve.jlsrc/CodeGeneration/ArrayODEGeneration.jlsrc/CodeGeneration/ArrayODEGenerationIR.jlsrc/CodeGeneration/CodeGeneration.jlsrc/CodeGeneration/DAEInitSolve.jlsrc/IR/OMJLIR.jlsrc/Precompile/ArraysKept.mosrc/backendAPI.jlsrc/globalConstants.jlsrc/precompile.jlsrc/util.jltest/Models/ArrayPath.motest/Models/IndexReduction.motest/arrayPathTests.jltest/indexReductionTests.jltest/initializationCausalRowsTests.jltest/precompileWorkloadTests.jltest/runtests.jl
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
| isempty(TUNABLE_PARAMETERS[]) || return decline("withTunableParameters (the ModelingToolkit path's tunable sets)") | ||
| local reduction::Module = Passes.IndexReduction | ||
| local (modelName::Union{Nothing, String}, modelCode::Union{String, Expr}) = try | ||
| local (solved::OMJLIR.Model, _) = @BACKEND_PERFLOG "[backendAPI] solvedInArrays" reduction.solvedInArrays(reduced) |
There was a problem hiding this comment.
Pass a copy of reduced to solvedInArrays. The scalar fallback reuses the same model.
This docstring and the docstring of solvedInArrays both say that reduced is not changed. solvedInArrays calls splitArrays!(model, view, ...) (ArraySolve.jl Line 254), and splitArrays! changes the model in place.
Suppose the array code then declines, for example on the 64-split limit, a failed check, or a NotHandled from ArrayODEGen. translate (Line 415) and translateReducedArrays (Line 686) then call scalarizedReducedModel(fm, reduced) on the changed model.
The worst case is a partial change. splitInner! replaces model.variables (ArraySlicing.jl Line 211-212) before slicedEquation runs. slicedReference can then throw NotHandled, for example "the whole array ... split along its inner dimensions" or "partly subscripted". The model is left with sliced variables and unsliced equations.
The scalar fallback then makes simulation code with references to variables that do not exist. _checkSimCodeBeforeCodegen then aborts translate. The model fails to translate, where the scalar path would have taken it before this PR.
Copy what the splits change, as knownValues does:
🛡️ Proposed fix
-
--- "a/src/backendAPI.jl" +++ "b/src/backendAPI.jl" @@ -649,7 +649,11 @@ isempty(TUNABLE_PARAMETERS[]) || return decline("withTunableParameters (the ModelingToolkit path's tunable sets)") local reduction::Module = Passes.IndexReduction local (modelName::Union{Nothing, String}, modelCode::Union{String, Expr}) = try - local (solved::OMJLIR.Model, _) = @BACKEND_PERFLOG "[backendAPI] solvedInArrays" reduction.solvedInArrays(reduced) + #= solvedInArrays splits arrays in place (splitArrays!): a copy, so the scalarized fallback reads `reduced` as reduced =# + local working::OMJLIR.Model = OMJLIR.Model(reduced.name, deepcopy(reduced.variables), deepcopy(reduced.continuous), + reduced.clocked, reduced.baseClocks, reduced.functions, reduced.initialMode, + reduced.modes, reduced.transitions, reduced.frontendModel) + local (solved::OMJLIR.Model, _) = @BACKEND_PERFLOG "[backendAPI] solvedInArrays" reduction.solvedInArrays(working) @BACKEND_PERFLOG "[backendAPI] array code (after index reduction)" CodeGeneration.ArrayODEGen.generateArrayODECode(solved, name; functions = functionList) catch err isFatal(err) && rethrow()
Also correct the solvedInArrays docstring. It changes model in place when it splits.
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changelocal (solved::OMJLIR.Model, _) = @BACKEND_PERFLOG "[backendAPI] solvedInArrays" reduction.solvedInArrays(reduced) #= solvedInArrays splits arrays in place (splitArrays!): a copy, so the scalarized fallback reads `reduced` as reduced =# local working::OMJLIR.Model = OMJLIR.Model(reduced.name, deepcopy(reduced.variables), deepcopy(reduced.continuous), reduced.clocked, reduced.baseClocks, reduced.functions, reduced.initialMode, reduced.modes, reduced.transitions, reduced.frontendModel) local (solved::OMJLIR.Model, _) = @BACKEND_PERFLOG "[backendAPI] solvedInArrays" reduction.solvedInArrays(working)
| end | ||
| #= EngineV6's piston speed at top dead center, zero to round-off, flipped a relation in every pass of the event | ||
| iteration while each solve stepped again =# | ||
| (iteration == 1 && step <= 1.0e-5) && return true |
There was a problem hiding this comment.
After a fresh Jacobian, the first-step early return leaves the equations at a perturbed point.
_blockJacobian! calls residual!(s.rp, xp, args...) once per color. Each call places the perturbed xp into du and a and computes the loop's other unknowns there.
Consider a call where full || !s.factored computes the Jacobian at Line 164 and the first step is already at round-off. Line 178 then returns true without a further evaluation at x. blockSolve! skips its final residual! when ok && z === nothing && !mode (Line 140).
Result: du and a keep the values of the last color's perturbed point, offset by sqrt(eps) * max(1, |x|), about 1.5e-8 relative. This breaks the docstring contract, "the last evaluation at the returned iterate". The RHS gets noise at the scale of a 1e-8 tolerance. The trigger is common: the first solve from good start values, the retry with full = true, or a factorization dropped after a singular Jacobian.
The other exits are safe. They either evaluate at x after the Jacobian or return false.
🐛 Proposed fix
-
--- "a/src/CodeGeneration/ArrayBlockSolve.jl" +++ "b/src/CodeGeneration/ArrayBlockSolve.jl" @@ -175,7 +175,14 @@ end #= EngineV6's piston speed at top dead center, zero to round-off, flipped a relation in every pass of the event iteration while each solve stepped again =# - (iteration == 1 && step <= 1.0e-5) && return true + if iteration == 1 && step <= 1.0e-5 + #= a Jacobian of this call evaluated last at its perturbed points: the equations back at the iterate =# + if fresh + residual!(r, x, args...) + s.residuals += 1 + end + return true + end x .-= dx residual!(r, x, args...) s.residuals += 1
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested change(iteration == 1 && step <= 1.0e-5) && return true if iteration == 1 && step <= 1.0e-5 #= a Jacobian of this call evaluated last at its perturbed points: the equations back at the iterate =# if fresh residual!(r, x, args...) s.residuals += 1 end return true end
| local nBlocks::Int = _solveCausalBlocks!(u0, rhsFunc, p_vec, eq_idx, eq_target, extra_residuals, causalVars, sparsity; | ||
| tol = tol, maxiter = min(maxiter, 50)) |
There was a problem hiding this comment.
🔎 Supported by static analysis
-
Script executed:
rg -n -F -e '_solveCausalBlocks!' -e '_solveCausalRows!' -e '_fallback' -e 'try' -e 'catch' -- src/CodeGeneration/DAEInitSolve.jl || test "$?" -eq 1 rg -n -F -e 'function _fallback' -e 'macro _fallback' -- src/ || test "$?" -eq 1 ast-grep outline src/CodeGeneration/DAEInitSolve.jl --view expanded
Repository: OpenModelica/OMBackend.jl
Length of output: 3928
Script executed:
sed -n '150,200p' src/errorPolicy.jl echo '---- DAEInitSolve 216-260 ----' sed -n '216,260p' src/CodeGeneration/DAEInitSolve.jl echo '---- DAEInitSolve 315-400 ----' sed -n '315,400p' src/CodeGeneration/DAEInitSolve.jl rg -n -F -e 'function _solveDAEPhase!' -e 'function _initIncidence' -- src/ || test "$?" -eq 1
Repository: OpenModelica/OMBackend.jl
Length of output: 8474
Script executed:
sed -n '716,760p' src/CodeGeneration/DAEInitSolve.jl echo '---- 470-510 ----' sed -n '470,510p' src/CodeGeneration/DAEInitSolve.jl echo '---- nBlocks uses ----' rg -n -F -e 'nBlocks' -- src/ || test "$?" -eq 1
Repository: OpenModelica/OMBackend.jl
Length of output: 8476
Guard the new blockwise pass against exceptions.
_solveCausalBlocks! runs without a try at its call site. An exception from rhsFunc, _initIncidence, or _solveDAEPhase! propagates out of _solveDAEInitializationErased!, and the phases after the call do not run. The earlier _solveCausalRows! already guards its probes with _fallback for the same reason. Catch the exception here too, so the phases below can run.
_fallback rethrows fatal exceptions and exceptions it classifies as bugs, so programming errors still surface. Other exceptions are logged once per site. Each block solves on a trial copy, so a failed block leaves u0 as it was after the last converged block. The return value of nBlocks only feeds the trace message, so returning 0 has no effect on control flow.
Proposed fix
-
--- "a/src/CodeGeneration/DAEInitSolve.jl" +++ "b/src/CodeGeneration/DAEInitSolve.jl" @@ -481,8 +481,13 @@ #= then the blocks in causal order, the initialization equations' rows among them: each a smaller, better-posed Newton than the whole =# if sparsity !== nothing && !isempty(causalVars) && length(causalVars) == length(init_res_vec) - local nBlocks::Int = _solveCausalBlocks!(u0, rhsFunc, p_vec, eq_idx, eq_target, extra_residuals, causalVars, sparsity; - tol = tol, maxiter = min(maxiter, 50)) + local nBlocks::Int = try + _solveCausalBlocks!(u0, rhsFunc, p_vec, eq_idx, eq_target, extra_residuals, causalVars, sparsity; + tol = tol, maxiter = min(maxiter, 50)) + catch e + OMBackend._fallback(e, :solveCausalBlocks) + 0 + end OMBackend.envSwitch("OMBACKEND_INIT_TRACE") && println("[initblocks] ", nBlocks, " of ", length(causalVars), " unknowns solved block by block") end
Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changelocal nBlocks::Int = _solveCausalBlocks!(u0, rhsFunc, p_vec, eq_idx, eq_target, extra_residuals, causalVars, sparsity; tol = tol, maxiter = min(maxiter, 50)) local nBlocks::Int = try _solveCausalBlocks!(u0, rhsFunc, p_vec, eq_idx, eq_target, extra_residuals, causalVars, sparsity; tol = tol, maxiter = min(maxiter, 50)) catch e OMBackend._fallback(e, :solveCausalBlocks) 0 end
…blocks' init pass guarded - generateReducedArrayODETargetCode: solvedInArrays splits arrays in place, so it solves a copy and the scalarized fallback reads the reduced model as reduced (a split that throws part-way left it half split). - _blockNewton!: after a fresh Jacobian, a first step at round-off evaluates the equations at the iterate again (they were left at the last color's perturbed point). - The initialization's blocks pass: an exception at a point only it reaches goes on to the next phases. Assisted-by: Claude Opus 5.5
Models translated with their arrays kept now stay in array code after index reduction, including models whose components are declared separately (MSL EngineV6's six cylinders), and that array code is faster than the scalar path on EngineV6.
solvedInArrays), an OMJLIR front half for ArrayODEGen; slicing of arrays the plan treats per element; records, functions and if-equations in array code.rollComponents!,OMBACKEND_ROLL_COMPONENTS, now on by default): separately declared components of one class rolled into arrays over the instances; elements keep their names.ArrayBlockSolve.jl), so the solver integrates the states only.invariants!), crossing functions from what the relations need (crossings!).src/Precompile/ArraysKept.mo). EngineV6 builds in 76 s in a fresh process (was 142 s).EngineV6 (MSL 4.1.0), warm: 0.12 s for 0.25 s (scalar path 0.20 s); 1 s at tolerance 1e-8: 4.6 s (scalar path 16.6 s), load.w matching the scalar path to 1.5e-8.
Coverage-relevant defaults: array index reduction with rolling, and models with large loops plus events or initial equations, now take the array code. MSL 4.1.0 coverage with rolling on, locally (published run in parentheses): validate 503 (500), simulate 519 (516); newly validating EngineV6, Spice3 Inverter, HysteresisModelComparison, FluidHeatFlow TwoMass, Fluid BranchingDynamicPipes; Rectifier6pulseFFT and Rectifier12pulseFFT killed at the 8 GB worker limit (scalar path, the known FFT simulation memory).
test/runtests.jl1080/1080; OM.jl suite (all groups) 1773 pass, 0 failures, 12 min. Precompile ~33 s longer (the workload). OMBackend 0.8.3.Supersedes #93 (closed): the same work, squashed into one commit.
🤖 Generated with Claude Code