Skip to content

feat(mixedprecision): POP precision tuning integration#531

Merged
Ravenwater merged 8 commits intomainfrom
feat/pop-implementation
Mar 2, 2026
Merged

feat(mixedprecision): POP precision tuning integration#531
Ravenwater merged 8 commits intomainfrom
feat/pop-implementation

Conversation

@Ravenwater
Copy link
Contributor

@Ravenwater Ravenwater commented Mar 1, 2026

Summary

  • Add POP (Precision-Optimized Programs) static bit-level precision analysis to the mixed-precision SDK
  • Given accuracy requirements on program outputs, determines minimum bits needed at each variable/intermediate using forward/backward error transfer functions solved as an LP
  • Five-phase implementation: transfer functions, expression graph with iterative fixpoint, embedded simplex LP solver, carry-bit refinement, and mixed-precision C++ code generator
  • Integrates with existing range_analyzer (new ufp() alias), TypeAdvisor (new recommendForNsb() method), and PrecisionConfigGenerator output format
  • 9 new headers under include/sw/universal/mixedprecision/, 8 test files under mixedprecision/pop/, all passing on both gcc and clang

Reference

Dorra Ben Khalifa, "Fast and Efficient Bit-Level Precision Tuning," PhD thesis, Universite de Perpignan, 2021.

Test plan

  • Build with gcc, all 8 tests pass via ctest
  • Build with clang 18, all 8 tests pass via ctest
  • CI pipeline validates build

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a mixed‑precision POP precision‑tuning framework: graph-based analysis, LP/ILP solvers (embedded + optional backend), carry‑bit refinement, type recommendation and header/code generation.
    • Build option to enable the POP component (auto‑enabled when the mixed‑precision SDK is on).
  • Tests

    • Comprehensive unit and end‑to‑end tests exercising analysis, solvers, transfer functions, UFP, carry refinement, codegen, and full workflows.
  • Documentation

    • New developer guide and expanded design/implementation documentation for POP.

Add static bit-level precision analysis based on the POP (Precision-
Optimized Programs) method. Given accuracy requirements on program
outputs, POP determines minimum bits needed at each variable using
forward/backward error transfer functions solved as an LP.

Five-phase implementation:
- Phase 1: constexpr transfer functions and UFP computation
- Phase 2: ExprGraph DAG with iterative fixpoint analysis
- Phase 3: embedded simplex LP solver + PopSolver constraint generator
- Phase 4: carry-bit refinement via policy iteration
- Phase 5: mixed-precision C++ code generator + umbrella header

Integrates with existing range_analyzer (ufp() alias), TypeAdvisor
(recommendForNsb method), and PrecisionConfigGenerator output format.
Optional GLPK binding for ILP on larger problems.

8 test files, all passing on both gcc and clang.

Reference: Dorra Ben Khalifa, "Fast and Efficient Bit-Level Precision
Tuning," PhD thesis, Universite de Perpignan, 2021.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 1, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a POP mixed‑precision subsystem: expression graph, transfer/UFP utilities, embedded Simplex and optional GLPK LP solvers, POP LP solver, carry‑bit iterative refinement, code generation, CMake wiring, TypeAdvisor/range_analyzer bridges, documentation, and a comprehensive test suite.

Changes

Cohort / File(s) Summary
Build Configuration
CMakeLists.txt, mixedprecision/pop/CMakeLists.txt
Adds UNIVERSAL_BUILD_MIXEDPRECISION_POP option, auto-enables it when UNIVERSAL_BUILD_MIXEDPRECISION_SDK is ON, and wires mixedprecision/pop into the build.
Expression Graph & Analysis
include/sw/universal/mixedprecision/expression_graph.hpp
Adds DAG-based ExprGraph/ExprNode, node metadata (lo/hi/ufp/nsb/carry), forward/backward precision propagation, require/analyze API, and reporting/type recommendation hooks.
Transfer & UFP Utilities
include/sw/universal/mixedprecision/transfer.hpp, include/sw/universal/mixedprecision/ufp.hpp, include/sw/universal/utility/range_analyzer.hpp
Adds forward/backward transfer functions and precision_info, UFP computation helpers and analyzer bridge, plus range_analyzer::ufp() convenience accessor.
LP Solvers
include/sw/universal/mixedprecision/simplex.hpp, include/sw/universal/mixedprecision/glpk_solver.hpp
Introduces header-only SimplexSolver and LPStatus, plus optional GlpkSolver wrapper (guarded by UNIVERSAL_HAS_GLPK) exposing a consistent LP API.
POP Solver
include/sw/universal/mixedprecision/pop_solver.hpp
Adds PopSolver to build and solve LP/ILP from an ExprGraph, write integer NSB results back into the graph, and report total/status.
Carry Refinement
include/sw/universal/mixedprecision/carry_analysis.hpp
Adds CarryAnalyzer implementing iterative carry‑bit refinement using LP solves, convergence tracking, and reporting.
Codegen & Umbrella Header
include/sw/universal/mixedprecision/codegen.hpp, include/sw/universal/mixedprecision/pop.hpp
Adds PopCodeGenerator (header/report/example generation) and pop.hpp umbrella documenting phases and aggregating headers.
TypeAdvisor Extension
include/sw/universal/utility/type_advisor.hpp
Adds TypeAdvisor::recommendForNsb(int nsb, double lo, double hi) to map NSB+range → concrete type recommendation.
POP Sources & Tests CMake
mixedprecision/pop/CMakeLists.txt, CMakeLists.txt
Globs POP sources and wires them into project build via existing compile helper.
Tests
mixedprecision/pop/test_ufp.cpp, .../test_transfer.cpp, .../test_simplex.cpp, .../test_pop_solver.cpp, .../test_expression_graph.cpp, .../test_complete_workflow.cpp, .../test_codegen.cpp, .../test_carry_analysis.cpp
Adds comprehensive tests for UFP, transfer functions, Simplex/GLPK, PopSolver, ExprGraph, carry refinement, codegen, and end‑to‑end workflows.
CI Scope
.github/workflows/conventional-commits.yml
Adds mixedprecision scope to conventional-commit validation.
Documentation
docs/design/pop-developer-guide.md, docs/design/pop-precision-tuning.md
Adds developer guide and expands/rewrites POP precision‑tuning docs with implementation details and examples.

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant RangeAnalyzer as RangeAnalyzer
    participant ExprGraph as ExprGraph
    participant PopSolver as PopSolver
    participant CarryAnalyzer as CarryAnalyzer
    participant PopCodeGenerator as PopCodeGenerator
    participant Output as Output

    User->>RangeAnalyzer: profile inputs (ranges, ufp)
    RangeAnalyzer-->>ExprGraph: attach range/ufp metadata
    User->>ExprGraph: build graph (constants, vars, ops)
    User->>ExprGraph: analyze() (forward/backward fixpoint)
    ExprGraph-->>User: nsb_forward/nsb_backward computed
    User->>PopSolver: solve(ExprGraph)
    PopSolver->>PopSolver: build LP (transfer constraints, mins)
    PopSolver-->>ExprGraph: write nsb_final
    User->>CarryAnalyzer: refine(ExprGraph)
    CarryAnalyzer->>PopSolver: iterative solves with updated carry flags
    PopSolver-->>CarryAnalyzer: LP results for validation
    CarryAnalyzer-->>ExprGraph: update carry flags
    User->>PopCodeGenerator: generateHeader/Report/ExampleCode
    PopCodeGenerator-->>Output: C++ header, report, example code
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Poem

🐰 I hopped through nodes with nimble paws,
Solved LP mazes, checked for flaws,
I chased the carries, bit by bit,
Emitted types and code to fit —
A tiny rabbit's POP applause!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.78% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(mixedprecision): POP precision tuning integration' clearly and specifically describes the main change: adding POP (Precision-Optimized Programs) precision tuning to the mixed-precision module.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/pop-implementation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Ravenwater Ravenwater changed the title feat(mixedprecision): POP precision tuning integration feat: POP precision tuning integration Mar 1, 2026
@Ravenwater Ravenwater marked this pull request as draft March 1, 2026 20:41
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (2)
mixedprecision/pop/test_carry_analysis.cpp (1)

34-37: Consider using CarryAnalyzer's max_iterations parameter instead of hardcoded 10.

The check iters >= 10 assumes the default max_iterations is 10. If CarryAnalyzer::refine has a configurable limit, consider passing it explicitly and checking against that value for robustness.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mixedprecision/pop/test_carry_analysis.cpp` around lines 34 - 37, The test
currently treats convergence as iters >= 10; instead use the CarryAnalyzer's
configured max iteration value and/or pass an explicit limit into
CarryAnalyzer::refine rather than hardcoding 10. Update the check to compare
iters against the analyzer's max_iterations (or the variable you pass when
calling CarryAnalyzer::refine) and, if you can, call refine with an explicit
max_iterations argument so the test and the analyzer use the same source of
truth (referencing CarryAnalyzer and its refine method/max_iterations member).
include/sw/universal/utility/type_advisor.hpp (1)

178-188: Align NSB accuracy evaluation with the existing error model.

rel_err/AccuracyRequirement are computed, but accuracy is decided via fraction_bits >= nsb. Reusing evaluateAccuracy() keeps this API consistent with the rest of TypeAdvisor.

♻️ Suggested consistency update
-            rec.meets_accuracy = (type.fraction_bits >= nsb);
+            rec.meets_accuracy = evaluateAccuracy(type, acc);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/sw/universal/utility/type_advisor.hpp` around lines 178 - 188, The
accuracy decision currently sets rec.meets_accuracy = (type.fraction_bits >=
nsb) which bypasses the existing error-model API; instead construct the
AccuracyRequirement acc (already created from rel_err) and call
evaluateAccuracy(type, acc, /*maybe other params*/ ) to set rec.meets_accuracy
so it uses the same accuracy logic as the rest of TypeAdvisor (replace the
fraction_bits >= nsb check with evaluateAccuracy(...) for TypeRecommendation rec
in the loop that also calls evaluateRange()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@include/sw/universal/mixedprecision/codegen.hpp`:
- Around line 200-206: The code computes a local rounded-up bit width named
total_bits (using nsb) then discards it and emits sw::universal::cfloat using
rec.type.total_bits; either remove the dead local calculation or use it. Fix by
replacing rec.type.total_bits with the locally computed total_bits in the
fallback return expression (so the return becomes "sw::universal::cfloat<" +
std::to_string(total_bits) + "," + std::to_string(exp_bits) + ">"), or if the
local total_bits is truly unnecessary, delete the total_bits and
(void)total_bits lines; update any related comments accordingly.

In `@include/sw/universal/mixedprecision/glpk_solver.hpp`:
- Around line 116-118: The get_value(int var) method currently returns
solution_[var] without checking bounds; add the same bounds check used in
SimplexSolver (assert(var >= 0 && var < static_cast<int>(solution_.size())))
before indexing solution_ to prevent out-of-bounds access, ensuring behavior and
safety of get_value in the same class as the other solver methods.
- Around line 26-32: GlpkSolver currently owns a raw glp_prob* (prob_) but lacks
copy/move special members; to follow the Rule of Five, explicitly delete the
copy constructor and copy assignment operator and explicitly default or
implement the move constructor and move assignment operator so ownership is
transferred safely (e.g., set moved-from prob_ to nullptr and preserve nvars_),
keeping the destructor calling glp_delete_prob(prob_); update class GlpkSolver
to declare GlpkSolver(const GlpkSolver&) = delete; GlpkSolver& operator=(const
GlpkSolver&) = delete; GlpkSolver(GlpkSolver&&) noexcept = default (or implement
to swap/prod nulling of prob_); GlpkSolver& operator=(GlpkSolver&&) noexcept =
default (or implement similarly).

In `@include/sw/universal/mixedprecision/pop_solver.hpp`:
- Around line 24-29: The code unconditionally uses SimplexSolver while including
glpk_solver.hpp when UNIVERSAL_HAS_GLPK is defined, causing a compile error;
update the header to provide a unified solver type or conditionally instantiate
the correct solver: either add a type alias (e.g., using MixedPrecisionSolver =
SimplexSolver or using MixedPrecisionSolver = GlpkSolver) when including the
respective header, or wrap the instantiation at line 46 (the use of
SimplexSolver) in `#ifdef` UNIVERSAL_HAS_GLPK / `#else` / `#endif` and
create/instantiate the appropriate solver class (GlpkSolver vs SimplexSolver) so
the symbol referenced (SimplexSolver or a new MixedPrecisionSolver alias) always
resolves.

In `@include/sw/universal/mixedprecision/simplex.hpp`:
- Around line 91-98: The objective_value() can return stale obj_value_ after
non-optimal runs; ensure obj_value_ is reset to a sentinel (e.g. NaN or a
distinct invalid value) whenever a solve() exits without LPStatus::Optimal or at
the start of solve(); update all exit paths in solve(), including the
early-return branch (when m==0 || n==0) and the other non-optimal return points
referenced around the mentioned ranges, to set obj_value_ to the sentinel and
keep status_ accurate so objective_value() cannot expose previous run values.
- Around line 76-82: add_le_constraint currently negates a <= into >= which can
produce a negative RHS and cause solve() to seed artificials from RHS, breaking
the initial basis; fix add_le_constraint (and the analogous add_eq_constraint
block) to ensure the RHS handed to add_ge_constraint is non-negative before
calling it: after negating coeffs and rhs, if rhs_out (the -rhs) is negative,
multiply both coeffs and rhs_out by -1 to flip the inequality back to a >= with
a non-negative RHS (or alternatively implement <= by adding a slack variable
instead of negating), so that add_ge_constraint always receives RHS >= 0 and
solve() will not create an invalid starting basis.

In `@include/sw/universal/mixedprecision/ufp.hpp`:
- Line 33: The range overload compute_ufp(lo, hi) must mirror the scalar
defensive check: if lo==0.0 && hi==0.0 return std::numeric_limits<int>::min()
(same sentinel used by compute_ufp(x)) so PopSolver won't receive INT_MIN from
an unguarded zero range; either add that early return inside compute_ufp(double
lo,double hi) or ensure callers in PopSolver verify ranges are not both zero
before calling compute_ufp. Update the compute_ufp range overload (and/or the
call sites in PopSolver) to perform this explicit zero-range check to keep
sentinel handling consistent.

In `@include/sw/universal/utility/type_advisor.hpp`:
- Around line 206-213: The fallback currently uses types_.back() which may not
be the largest type; change the fallback to select the largest/safest type by
scanning the types_ container (e.g., using std::max_element(types_.begin(),
types_.end(), comparator)) and compare the type size field (use the appropriate
member on your Type struct such as total_bits, bit_width, or width) to pick the
true largest type, then assign best.type to that selected largest type and
update best.estimated_energy = largest.energy_per_fma / 1.5 and the other best.*
fields as currently done.

---

Nitpick comments:
In `@include/sw/universal/utility/type_advisor.hpp`:
- Around line 178-188: The accuracy decision currently sets rec.meets_accuracy =
(type.fraction_bits >= nsb) which bypasses the existing error-model API; instead
construct the AccuracyRequirement acc (already created from rel_err) and call
evaluateAccuracy(type, acc, /*maybe other params*/ ) to set rec.meets_accuracy
so it uses the same accuracy logic as the rest of TypeAdvisor (replace the
fraction_bits >= nsb check with evaluateAccuracy(...) for TypeRecommendation rec
in the loop that also calls evaluateRange()).

In `@mixedprecision/pop/test_carry_analysis.cpp`:
- Around line 34-37: The test currently treats convergence as iters >= 10;
instead use the CarryAnalyzer's configured max iteration value and/or pass an
explicit limit into CarryAnalyzer::refine rather than hardcoding 10. Update the
check to compare iters against the analyzer's max_iterations (or the variable
you pass when calling CarryAnalyzer::refine) and, if you can, call refine with
an explicit max_iterations argument so the test and the analyzer use the same
source of truth (referencing CarryAnalyzer and its refine method/max_iterations
member).

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5c1787 and e882198.

📒 Files selected for processing (21)
  • CMakeLists.txt
  • include/sw/universal/mixedprecision/carry_analysis.hpp
  • include/sw/universal/mixedprecision/codegen.hpp
  • include/sw/universal/mixedprecision/expression_graph.hpp
  • include/sw/universal/mixedprecision/glpk_solver.hpp
  • include/sw/universal/mixedprecision/pop.hpp
  • include/sw/universal/mixedprecision/pop_solver.hpp
  • include/sw/universal/mixedprecision/simplex.hpp
  • include/sw/universal/mixedprecision/transfer.hpp
  • include/sw/universal/mixedprecision/ufp.hpp
  • include/sw/universal/utility/range_analyzer.hpp
  • include/sw/universal/utility/type_advisor.hpp
  • mixedprecision/pop/CMakeLists.txt
  • mixedprecision/pop/test_carry_analysis.cpp
  • mixedprecision/pop/test_codegen.cpp
  • mixedprecision/pop/test_complete_workflow.cpp
  • mixedprecision/pop/test_expression_graph.cpp
  • mixedprecision/pop/test_pop_solver.cpp
  • mixedprecision/pop/test_simplex.cpp
  • mixedprecision/pop/test_transfer.cpp
  • mixedprecision/pop/test_ufp.cpp

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Ravenwater Ravenwater changed the title feat: POP precision tuning integration feat(mixedprecision): POP precision tuning integration Mar 1, 2026
@coveralls
Copy link

coveralls commented Mar 1, 2026

Pull Request Test Coverage Report for Build 22577154792

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 84.245%

Files with Coverage Reduction New Missed Lines %
include/sw/universal/number/posit1/specialized/posit_16_1.hpp 1 80.58%
include/sw/universal/verification/posit_test_suite_randoms.hpp 5 50.45%
Totals Coverage Status
Change from base Build 22550260168: -0.01%
Covered Lines: 39500
Relevant Lines: 46887

💛 - Coveralls

@Ravenwater Ravenwater self-assigned this Mar 1, 2026
- simplex.hpp: add ConstraintKind enum, RHS normalization for negative
  values, reset state on solve entry, NaN for non-optimal objective
- pop_solver.hpp: add PopLPSolver type alias for conditional solver
- glpk_solver.hpp: add Rule of 5 (delete copy, implement move), bounds
  check on get_value
- codegen.hpp: fix dead code in universal_type_string fallback
- ufp.hpp: add zero-range sentinel check
- type_advisor.hpp: use max_element for fallback instead of types_.back()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
include/sw/universal/mixedprecision/codegen.hpp (2)

164-166: Consider documenting the lifetime requirement for graph_.

The graph_ member is stored by reference, which means the caller must ensure the ExprGraph outlives the PopCodeGenerator instance. This is a reasonable design choice for a generator, but a brief comment would clarify the contract.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/sw/universal/mixedprecision/codegen.hpp` around lines 164 - 166, The
member graph_ in PopCodeGenerator is stored as a reference to ExprGraph, so
callers must ensure the referenced ExprGraph outlives the PopCodeGenerator; add
a brief comment next to the declaration of graph_ (and optionally above the
class) stating this lifetime requirement and consequence, e.g., "graph_ is a
reference; ExprGraph must outlive PopCodeGenerator", and leave advisor_
untouched.

16-17: Include paths should use the sw/ prefix.

The includes should follow the repository's established pattern with the sw/ prefix:

♻️ Suggested fix
-#include <universal/mixedprecision/expression_graph.hpp>
-#include <universal/utility/type_advisor.hpp>
+#include <sw/universal/mixedprecision/expression_graph.hpp>
+#include <sw/universal/utility/type_advisor.hpp>

As per coding guidelines: "Include Universal header-only library types using the pattern '#include <universal/number/[type]/[type].hpp>'" - the sw/ prefix is part of the standard include structure in this repository.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/sw/universal/mixedprecision/codegen.hpp` around lines 16 - 17, Update
the include directives in codegen.hpp to use the repository's standard `sw/`
include prefix: replace `#include
<universal/mixedprecision/expression_graph.hpp>` and `#include
<universal/utility/type_advisor.hpp>` with their `sw/`-prefixed equivalents so
they follow the project's include pattern; ensure the new includes reference the
same headers (expression_graph.hpp and type_advisor.hpp) but with the `sw/` path
prefix.
include/sw/universal/mixedprecision/simplex.hpp (2)

28-28: Unused include can be removed.

<iostream> is included but the SimplexSolver class does not use any stream I/O. Removing it reduces header bloat.

♻️ Suggested fix
 `#include` <limits>
 `#include` <algorithm>
 `#include` <cassert>
-#include <iostream>
 
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/sw/universal/mixedprecision/simplex.hpp` at line 28, Remove the
unused `#include` <iostream> from the header (mixedprecision/simplex.hpp); the
SimplexSolver class and related declarations do not perform stream I/O, so
delete the include to reduce header bloat and then rebuild to ensure no compile
errors from other missing headers.

116-117: Big-M value may need tuning for edge cases.

The hardcoded M = 1e6 works for typical precision tuning problems but may be too small for LPs with large coefficient magnitudes, or too large for problems with very small coefficients (causing numerical instability). This is an inherent limitation of the Big-M method.

For broader applicability, consider making this configurable or documenting the constraint.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/sw/universal/mixedprecision/simplex.hpp` around lines 116 - 117, The
hardcoded Big-M value (double M = 1e6) can cause numeric issues; change M from a
fixed literal to a configurable parameter or compute it adaptively: add a
parameter or member (e.g., double bigM or setBigM) to the class or function that
constructs/solves the LP in simplex.hpp (refer to symbol M and the surrounding
simplex solver entrypoint), validate the provided value (positive and within a
safe range), and if not provided compute a heuristic based on input coefficient
magnitudes (e.g., max absolute coefficient times a safety factor) and document
the new parameter in the API/comments so callers can tune it for edge cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@include/sw/universal/mixedprecision/codegen.hpp`:
- Around line 70-86: Guard against division by zero when computing savings:
after the loop that accumulates total_bits and fp64_total over graph_.nodes()
(where advisor_.recommendForNsb is called for each node and nsb_final is used),
check whether fp64_total is zero before computing savings = (1.0 - total_bits /
fp64_total) * 100.0; if fp64_total == 0.0, produce a sensible fallback (e.g.,
set savings to 0.0 or emit "N/A"/skip the percentage) and adjust the subsequent
ss output lines that print Total bits and Bit savings so the report remains
well-formed when graph_.nodes() is empty.

---

Nitpick comments:
In `@include/sw/universal/mixedprecision/codegen.hpp`:
- Around line 164-166: The member graph_ in PopCodeGenerator is stored as a
reference to ExprGraph, so callers must ensure the referenced ExprGraph outlives
the PopCodeGenerator; add a brief comment next to the declaration of graph_ (and
optionally above the class) stating this lifetime requirement and consequence,
e.g., "graph_ is a reference; ExprGraph must outlive PopCodeGenerator", and
leave advisor_ untouched.
- Around line 16-17: Update the include directives in codegen.hpp to use the
repository's standard `sw/` include prefix: replace `#include
<universal/mixedprecision/expression_graph.hpp>` and `#include
<universal/utility/type_advisor.hpp>` with their `sw/`-prefixed equivalents so
they follow the project's include pattern; ensure the new includes reference the
same headers (expression_graph.hpp and type_advisor.hpp) but with the `sw/` path
prefix.

In `@include/sw/universal/mixedprecision/simplex.hpp`:
- Line 28: Remove the unused `#include` <iostream> from the header
(mixedprecision/simplex.hpp); the SimplexSolver class and related declarations
do not perform stream I/O, so delete the include to reduce header bloat and then
rebuild to ensure no compile errors from other missing headers.
- Around line 116-117: The hardcoded Big-M value (double M = 1e6) can cause
numeric issues; change M from a fixed literal to a configurable parameter or
compute it adaptively: add a parameter or member (e.g., double bigM or setBigM)
to the class or function that constructs/solves the LP in simplex.hpp (refer to
symbol M and the surrounding simplex solver entrypoint), validate the provided
value (positive and within a safe range), and if not provided compute a
heuristic based on input coefficient magnitudes (e.g., max absolute coefficient
times a safety factor) and document the new parameter in the API/comments so
callers can tune it for edge cases.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a71688 and ecf2433.

📒 Files selected for processing (6)
  • include/sw/universal/mixedprecision/codegen.hpp
  • include/sw/universal/mixedprecision/glpk_solver.hpp
  • include/sw/universal/mixedprecision/pop_solver.hpp
  • include/sw/universal/mixedprecision/simplex.hpp
  • include/sw/universal/mixedprecision/ufp.hpp
  • include/sw/universal/utility/type_advisor.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • include/sw/universal/utility/type_advisor.hpp
  • include/sw/universal/mixedprecision/glpk_solver.hpp

- Add pop-developer-guide.md with step-by-step workflow, complete code
  examples (determinant, dot product with profiling), API reference,
  and architecture overview
- Update pop-precision-tuning.md to reflect actual implementation:
  replace proposed APIs with real signatures, add header structure
  table, build config, test suite, and integration details

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/design/pop-developer-guide.md`:
- Line 346: Update the thesis reference string so the institution name uses the
correct French spelling: replace "Universite de Perpignan" with "Université de
Perpignan" in the citation line (the Dorra Ben Khalifa thesis reference).
- Line 61: The markdown table cell containing the example snippet `| \`z = |x|\`
|` needs its inner pipe characters escaped so the table doesn't get parsed as
extra columns; locate the table cell with the code example (the `z = |x|`
snippet) and replace inner pipe characters with escaped pipes (e.g., `\|`) or
use the HTML entity `&#124;` (e.g., change `` `z = |x|` `` to `` `z = \|x\|` ``
or `` `z = &#124;x&#124;` ``) so the table renders stably.
- Around line 103-104: The example uses a mismatched variable name: the snippet
defines/uses node_id but calls g.get_node(id), which won't compile; update the
API example so the same identifier is used (e.g., call g.get_node(node_id) or
rename node_id to id) so ExprNode& node = g.get_node(...) references the
correctly named variable and compiles.
- Around line 155-162: The example omits profiling of ra_y before creating
g.variable("y", ra_y): add code to observe the appropriate samples into ra_y
(e.g., a loop like the ra_x loop that calls ra_y.observe(val) for its sample
set) so that ra_y has lo/hi/ufp computed before ExprGraph::variable("y", ra_y)
is called; keep the same style/comment that g.variable extracts lo, hi, ufp from
the analyzer (referencing ra_x, ra_y, ExprGraph::variable).

In `@docs/design/pop-precision-tuning.md`:
- Line 61: The table row for the absolute value transfer function uses unescaped
pipe characters in the `|x|` token which breaks the two-column table; update the
row for the expression `z = |x|` so the inner pipes are escaped (e.g., `\|x\|`)
or replaced with inline code/backticks that avoid raw pipes, keeping the
right-hand cell `nsb(z) = nsb(x)` unchanged; locate the table row containing `z
= |x|` and fix the markdown so the row renders as two columns.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ecf2433 and a2d35aa.

📒 Files selected for processing (2)
  • docs/design/pop-developer-guide.md
  • docs/design/pop-precision-tuning.md

- Fix variable name mismatch in API example (id -> node_id)
- Complete range_analyzer profiling example for ra_y
- Escape pipe characters in abs(x) table row
- Fix French spelling: Université de Perpignan

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/design/pop-precision-tuning.md`:
- Line 7: The citation line containing "Reference: Dorra Ben Khalifa, 'Fast and
Efficient Bit-Level Precision Tuning,' PhD thesis, Universite de Perpignan,
2021" has a French spelling error; replace "Universite de Perpignan" with
"Université de Perpignan" so the reference reads "...PhD thesis, Université de
Perpignan, 2021".

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2d35aa and 472b6cf.

📒 Files selected for processing (2)
  • docs/design/pop-developer-guide.md
  • docs/design/pop-precision-tuning.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/design/pop-developer-guide.md

Ravenwater and others added 3 commits March 2, 2026 07:51
When graph has no nodes, fp64_total is zero. Skip the "Bit savings"
line instead of producing inf/nan output.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Universite -> Université de Perpignan in all 10 thesis references.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
C++20 starts_with() is clearer and avoids scanning the full string.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Ravenwater Ravenwater marked this pull request as ready for review March 2, 2026 13:01
@Ravenwater Ravenwater merged commit 401df41 into main Mar 2, 2026
31 of 32 checks passed
@Ravenwater Ravenwater deleted the feat/pop-implementation branch March 2, 2026 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants