Conversation
Signed-off-by: GitHub Actions <actions@github.com>
…ping to guard `mockturtle::emap`
Signed-off-by: GitHub Actions <actions@github.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces mockturtle::map with mockturtle::emap across technology mapping, adds fiction::missing_required_gates_exception and validate_required_gates(), converts many public string constexprs to auto and adds half‑adder gate/constants, updates Python bindings/docs/tests, and adjusts CLI, build, and CI settings. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant CLI
end
rect rgba(200,255,200,0.5)
participant TechMapping as TechnologyMappingImpl
end
rect rgba(255,200,200,0.5)
participant EMAP as mockturtle::emap
end
rect rgba(255,255,200,0.5)
participant Store
end
CLI->>TechMapping: call technology_mapping(params)
TechMapping->>TechMapping: validate_required_gates(params)
alt missing gates
TechMapping-->>CLI: throws missing_required_gates_exception
CLI->>CLI: catch and print exception message
else all gates present
TechMapping->>EMAP: invoke mockturtle::emap(ntk, lib, emap_params)
EMAP-->>TechMapping: mapping result & stats
TechMapping->>Store: extend with mapped tec_nt (on success)
CLI-->>CLI: report success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Signed-off-by: Marcel Walter <marcel.walter@tum.de>
Signed-off-by: GitHub Actions <actions@github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/fiction/types.hpp (1)
211-213: Probable alias bug: cube CDS uses SIQAD typecds_sidb_cell_clk_lyt_cube aliases charge_distribution_surface<sidb_cell_clk_lyt_siqad>, but should use sidb_cell_clk_lyt_cube.
-using cds_sidb_cell_clk_lyt_cube = charge_distribution_surface<sidb_cell_clk_lyt_siqad>; +using cds_sidb_cell_clk_lyt_cube = charge_distribution_surface<sidb_cell_clk_lyt_cube>;include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
316-319: Fixtechnology_mapping_stats::report()return type—Python bindings expect a stringThe Python binding at line 78 of
bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hppdefines__repr__asreturn stats.report(), which requiresreport()to return a string. The currentvoidreturn type will break this binding.Change
report()to returnstd::stringand aggregatemapper_stats.report()output usingstd::stringstream, following the pattern used by other stat classes in the codebase (e.g.,orthogonal_physical_design_stats,exact_physical_design_stats).
🧹 Nitpick comments (15)
bindings/mnt/pyfiction/test/algorithms/network_transformation/test_technology_mapping.py (1)
59-71: Consider importing and testing the specific exception type.The test correctly validates that an exception is raised with the appropriate message when no gates are enabled. However, it checks for
RuntimeErrorrather than the specificmissing_required_gates_exceptiontype.While pybind11's
py::register_exceptiontypically creates exceptions that inherit fromRuntimeError, you could make the test more precise by importing and checking for the specific exception type:from mnt.pyfiction import ( # ... existing imports ... missing_required_gates_exception, ) # In the test: with self.assertRaises(missing_required_gates_exception) as context: technology_mapping(network, params)This would catch cases where a different
RuntimeErroris raised for unrelated reasons.bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp (3)
10651-10656: Tighten wording and add a cross‑reference for discoverability.Small style tweak and a See also to the exception.
static const char *__doc_fiction_detail_technology_mapping_impl_validate_required_gates = -R"doc(Validate that the technology library contains the required gates for -the base network type. - -Throws: - missing_required_gates_exception if required gates are missing.)doc"; +R"doc(Validates that the technology library contains the gates required by +the base network type. + +Throws: + missing_required_gates_exception if required gates are missing. + +See also: + missing_required_gates_exception.)doc";
16977-16982: Clarify what the exception surfaces to help debugging.If the implementation includes the missing gate kinds in the message, reflect that here.
static const char *__doc_fiction_missing_required_gates_exception = -R"doc(Exception thrown when a technology mapping library does not contain -the required gates for the base network type.)doc"; +R"doc(Exception thrown when a technology mapping library does not contain +the required gates for the base network type. + +The exception message lists the missing gate kinds to aid debugging.)doc";If messages do not yet include the missing kinds, would you like a follow‑up change to add them?
21985-21986: LGTM: MUX parameter doc reads well.Optional: if
mapper_params(Line 21983) now refers specifically to emap’s parameter struct, consider clarifying that in its docstring.test/algorithms/network_transformation/technology_mapping.cpp (7)
229-234: MIG-only AOIM tests: confirm intentional backend reductionSwitching this case to mig_network only looks consistent with EMAP’s MAJ support. Please confirm dropping AIG/XAG/XMG here is intentional and not a coverage regression.
256-260: MIG-only 3‑input tests: verify intended scopeLimiting to mig_network makes sense for MAJ-focused coverage. If XMG is unsupported/irrelevant under EMAP, all good—otherwise consider retaining it for parity.
265-271: Use of all_supported_standard_functions is correctGood move to cover both 2‑ and 3‑input gates in one go. Optionally, assert some gate-type counts to ensure expected families are actually used.
288-314: AIG missing-gates tests are sound; re-init params within each SECTIONLooks correct. For clarity and isolation, consider constructing a fresh technology_mapping_params inside each SECTION to avoid accidental flag carry-over if sections change later.
-technology_mapping_params params{}; -SECTION("Missing INV") { params.and2 = true; /* ... */ } +SECTION("Missing INV") { + technology_mapping_params params{}; + params.and2 = true; + CHECK_THROWS_AS(technology_mapping(aig, params), missing_required_gates_exception); +}
315-350: XAG missing-gates matrix is completeINV/AND/XOR combinations covered well. Same suggestion to re-init params within each SECTION for robustness.
352-377: MIG missing-gates checks are correctINV/MAJ requirements validated. Consider local params per SECTION for consistency with other blocks.
379-411: Positive cases: add a basic correctness/health assertionSince you’re already constructing networks here, add a quick stats or equivalence check to ensure mapping ran and didn’t silently degrade:
- CHECK_NOTHROW(technology_mapping(aig, params)); + CHECK_NOTHROW(technology_mapping(aig, params)); + // Optional: sanity check via stats + technology_mapping_stats st{}; + (void)technology_mapping(aig, params, &st); + CHECK_FALSE(st.mapper_stats.mapping_error);include/fiction/types.hpp (2)
58-74: Prefer constexpr std::string_view for namesUsing inline constexpr auto with string literals deduces to char[N], which decays fine but is less explicit. std::string_view avoids decay pitfalls and is header‑only.
- inline constexpr auto aig_name = "AIG"; - inline constexpr auto xag_name = "XAG"; - inline constexpr auto mig_name = "MIG"; - inline constexpr auto tec_name = "TEC"; + inline constexpr std::string_view aig_name = "AIG"; + inline constexpr std::string_view xag_name = "XAG"; + inline constexpr std::string_view mig_name = "MIG"; + inline constexpr std::string_view tec_name = "TEC";
78-82: Make ntk_type_name a string_viewAlign return type to std::string_view for consistency with the name constants and to avoid pointer decay.
-template <class Ntk> -inline constexpr auto ntk_type_name = std::is_same_v<std::decay_t<Ntk>, aig_nt> ? aig_name : - std::is_same_v<std::decay_t<Ntk>, xag_nt> ? xag_name : - std::is_same_v<std::decay_t<Ntk>, mig_nt> ? mig_name : - std::is_same_v<std::decay_t<Ntk>, tec_nt> ? tec_name : - "?"; +template <class Ntk> +inline constexpr std::string_view ntk_type_name = + std::is_same_v<std::decay_t<Ntk>, aig_nt> ? aig_name : + std::is_same_v<std::decay_t<Ntk>, xag_nt> ? xag_name : + std::is_same_v<std::decay_t<Ntk>, mig_nt> ? mig_name : + std::is_same_v<std::decay_t<Ntk>, tec_nt> ? tec_name : + std::string_view{"?"};cli/cmd/logic/map.hpp (1)
136-141: Optional: print stats when verboseIf ps.mapper_params.verbose is set, consider printing st.mapper_stats.report() after mapping to mirror Mockturtle behavior.
include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
379-443: Validation step is correct; make header self-containedGate checks per base network are right. To avoid relying on transitive includes, add <type_traits> in this header since validate_required_gates uses std::is_same_v.
#include <sstream> -#include <stdexcept> -#include <string> +#include <stdexcept> +#include <string> +#include <type_traits>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hpp(1 hunks)bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp(5 hunks)bindings/mnt/pyfiction/test/algorithms/network_transformation/test_technology_mapping.py(2 hunks)cli/cmd/logic/map.hpp(3 hunks)docs/algorithms/network_transformation.rst(2 hunks)docs/changelog.rst(2 hunks)include/fiction/algorithms/network_transformation/technology_mapping.hpp(8 hunks)include/fiction/types.hpp(1 hunks)test/algorithms/network_transformation/technology_mapping.cpp(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T11:04:24.736Z
Learnt from: marcelwa
PR: cda-tum/fiction#854
File: cli/cmd/logic/src/abc.cpp:78-88
Timestamp: 2025-10-13T11:04:24.736Z
Learning: The `mockturtle::write_aiger` function in the mockturtle library supports writing all kinds of logic networks (AIG, XAG, MIG, TEC), not just AIG networks. No explicit conversion to AIG is required before calling this function.
Applied to files:
test/algorithms/network_transformation/technology_mapping.cpp
🧬 Code graph analysis (3)
bindings/mnt/pyfiction/test/algorithms/network_transformation/test_technology_mapping.py (2)
include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
technology_mapping(634-635)bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hpp (4)
technology_mapping(23-30)technology_mapping(23-23)technology_mapping(34-97)technology_mapping(34-34)
test/algorithms/network_transformation/technology_mapping.cpp (1)
include/fiction/algorithms/network_transformation/technology_mapping.hpp (4)
technology_mapping(634-635)all_supported_standard_functions(245-245)missing_required_gates_exception(33-36)missing_required_gates_exception(33-33)
cli/cmd/logic/map.hpp (3)
include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
technology_mapping(634-635)bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hpp (4)
technology_mapping(23-30)technology_mapping(23-23)technology_mapping(34-97)technology_mapping(34-34)cli/cmd/io/sqd.hpp (1)
s(48-108)
🔇 Additional comments (12)
bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hpp (1)
38-40: LGTM!The exception registration follows pybind11 conventions correctly. The placement before the parameter class bindings is appropriate, ensuring the exception type is available when the
technology_mappingfunction is called from Python.bindings/mnt/pyfiction/test/algorithms/network_transformation/test_technology_mapping.py (1)
14-14: LGTM!The import of
technology_mapping_paramsis necessary for the new test and correctly added to the existing import statement.docs/changelog.rst (2)
26-27: LGTM!The changelog entry accurately documents the switch from
mockturtle::maptomockturtle::emapand is appropriately categorized under the "Changed" section for algorithms.
41-42: LGTM!The changelog entry correctly documents the removal of the
--logic_sharingflag with clear rationale thatmockturtle::emapdoes not support it. The entry is appropriately placed in the "Removed > CLI" section.docs/algorithms/network_transformation.rst (2)
52-53: LGTM!The documentation correctly exposes the new
missing_required_gates_exceptionclass in the C++ API documentation using the appropriatedoxygenclassdirective. The placement in the Technology Mapping section is logical and consistent with the feature's purpose.
66-67: LGTM!The Python documentation correctly mirrors the C++ documentation by exposing the
missing_required_gates_exceptionclass using theautoclassdirective. This maintains symmetry between the C++ and Python API documentation.bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp (2)
21975-21976: LGTM: clear and consistent parameter description.
21936-21938: Remove unsupported "replaces map backend" claim; verify multi-output relevance.The codebase shows no evidence of a previous
mockturtle::mapbackend or migration context. While emap does support multi-output cells viaemap_params::map_multioutput = true, the suggested docstring addition conflates mockturtle's library design with this project's actual implementation. The function docstring should describe whattechnology_mappingdoes, not general emap capabilities, unless this function provably uses multi-output mapping.Likely an incorrect or invalid review comment.
test/algorithms/network_transformation/technology_mapping.cpp (1)
273-287: Name conservation check retained—LGTMThe MIG-only name preservation case is concise and effective.
cli/cmd/logic/map.hpp (1)
119-128: Warning on remapping tec_nt is helpfulGood proactive UX to warn when attempting to remap an already mapped network.
include/fiction/algorithms/network_transformation/technology_mapping.hpp (2)
27-37: New exception type is well-scopedClear, specific error for missing required gates per network family. Good addition.
605-607: EMAP call swap looks correctSwitch to mockturtle::emap with emap_params/stats is consistent with the PR objective.
# Conflicts: # cli/cmd/logic/map.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
622-632: Update documentation to mention the new exception.The documentation correctly references
mockturtle::emap, but it should also document that the function may throwmissing_required_gates_exceptionwhen the provided library lacks required gates for the input network type.Apply this diff to enhance the documentation:
/** * Performs technology mapping on the given network. Technology mapping is the process of replacing the gates in a * network with gates from a given technology library. This function utilizes `mockturtle::emap` to perform the * technology mapping. This function is a wrapper around that interface to provide a more convenient usage. * * @tparam Ntk Input logic network type. * @param ntk Input logic network. * @param params Technology mapping parameters. * @param pst Technology mapping statistics. * @return Mapped network exclusively using gates from the provided library. + * @throws missing_required_gates_exception if the technology library does not contain required gates for the base + * network type (e.g., AIG requires INV and AND; XAG requires INV, AND, and XOR; MIG requires INV and MAJ). */
🧹 Nitpick comments (2)
include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
431-439: Consider simplifying the comma-separated list construction.The manual loop to build the comma-separated string is correct but could be more concise.
Apply this diff to simplify using string operations:
- std::string missing_list{}; - for (std::size_t i = 0; i < missing_gates.size(); ++i) - { - missing_list += missing_gates[i]; - if (i < missing_gates.size() - 1) - { - missing_list += ", "; - } - } + std::ostringstream oss; + for (std::size_t i = 0; i < missing_gates.size(); ++i) + { + if (i > 0) oss << ", "; + oss << missing_gates[i]; + } + const std::string missing_list = oss.str();Note: This requires
#include <sstream>which is already included (line 19).cli/cmd/logic/src/map.cpp (1)
115-122: Robust mapping lambda; consider early-return for already-mapped networksThe new
perform_mappinglambda withNtkdeduction, warning onfiction::tec_nt, stats-basedmapping_errorcheck, andmissing_required_gates_exceptionhandling is a solid improvement in robustness: mapped networks are only stored when mapping succeeds and required gates are present.As an optional refinement, you could:
- Early-return when
Ntkisfiction::tec_ntto avoid invokingtechnology_mappingon an already mapped network and relying on a warning + potential mapper error.- Optionally surface more detail from
st.mapper_statsin the error path if that’s useful for users diagnosing mapping failures.These are polish-level improvements; the current behavior is functionally acceptable.
Also applies to: 126-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp(5 hunks)cli/cmd/logic/include/map.hpp(1 hunks)cli/cmd/logic/src/map.cpp(4 hunks)docs/changelog.rst(2 hunks)include/fiction/algorithms/network_transformation/technology_mapping.hpp(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T11:04:24.736Z
Learnt from: marcelwa
Repo: cda-tum/fiction PR: 854
File: cli/cmd/logic/src/abc.cpp:78-88
Timestamp: 2025-10-13T11:04:24.736Z
Learning: The `mockturtle::write_aiger` function in the mockturtle library supports writing all kinds of logic networks (AIG, XAG, MIG, TEC), not just AIG networks. No explicit conversion to AIG is required before calling this function.
Applied to files:
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hppinclude/fiction/algorithms/network_transformation/technology_mapping.hpp
📚 Learning: 2025-10-13T14:43:13.479Z
Learnt from: hibenj
Repo: cda-tum/fiction PR: 852
File: include/fiction/algorithms/physical_design/orthogonal_planar.hpp:825-826
Timestamp: 2025-10-13T14:43:13.479Z
Learning: In the fiction library, logic networks are required to have at least one primary input (PI). The `orthogonal_planar` algorithm in `include/fiction/algorithms/physical_design/orthogonal_planar.hpp` enforces this invariant with an assertion rather than handling the zero-PI case.
Applied to files:
include/fiction/algorithms/network_transformation/technology_mapping.hpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: 🐧 ubuntu-24.04-arm with clang++-15
- GitHub Check: 🐧 ubuntu-22.04 with g++-11
- GitHub Check: 🐍 windows-latest
- GitHub Check: 🐳 Build and publish Docker image
- GitHub Check: 🍎 macos-13 with g++-13
- GitHub Check: 🍎 macos-13 with clang++
- GitHub Check: 🍎 macos-13 with g++-14
- GitHub Check: Analyze python
- GitHub Check: 🪟 windows-2025 with v143 toolset
- GitHub Check: Analyze cpp
- GitHub Check: 🪟 windows-2025 with ClangCL toolset
- GitHub Check: 🪟 windows-2022 with ClangCL toolset
- GitHub Check: 🪟 windows-2022 with v143 toolset
- GitHub Check: 🛞 Wheels for windows-latest
- GitHub Check: 🛞 Wheels for ubuntu-24.04-arm
- GitHub Check: 🛞 Wheels for macos-14
- GitHub Check: 🛞 Wheels for ubuntu-24.04
- GitHub Check: 🛞 Wheels for macos-13
- GitHub Check: Coverage on ubuntu-24.04 with g++-13
- GitHub Check: 🚨 Clang-Tidy
🔇 Additional comments (15)
include/fiction/algorithms/network_transformation/technology_mapping.hpp (6)
53-55: Good documentation improvements.The clarifications for INV (inverter) and MUX (ITE) improve readability without changing functionality.
Also applies to: 132-132
335-338: Excellent additions to the run() method.The
[[nodiscard]]attribute ensures the returned mapped network is used, and the early validation provides clear error messages when required gates are missing from the library.
374-426: Robust validation logic for network-specific gate requirements.The validation correctly identifies required gates for each base network type (AIG, XAG, MIG) and provides clear error messages. This fail-fast approach prevents cryptic mapping errors.
605-605: emap migration properly implemented with correct parameter types.Verification confirms the code is correct: emap uses its own emap_params/emap_stats, and the codebase at lines 44 and 312 has already been updated to use
mockturtle::emap_paramsandmockturtle::emap_statsrespectively. The function call at line 605 passes the correct parameter types, enabling the multi-output cell support objective.
27-37: Exception implementation verified and approved.The
missing_required_gates_exceptionclass is well-designed and follows best practices. Verification confirms that thefmtlibrary is properly integrated in the project—it's bundled from the alice/mockturtle submodules and the CMake build system is correctly configured to prevent version conflicts. The exception's use offmt::formatis consistent with extensive usage throughout the codebase.
44-44: Verify API compatibility and field availability for emap_params.The web search confirms that
mockturtle::emap_paramsandmockturtle::map_paramsare not API-compatible and cannot be substituted for each other. The codebase has swapped them globally, which introduces several risks:
Missing field access: CLI code (
cli/cmd/logic/src/map.cpp:54, 78, 97) accessesmapper_params.verbose, but the emap_params documentation does not list this field. This may cause compilation or runtime errors.Cut enumeration default change: emap_params uses a default cut limit of 20, whereas map_params uses 49. This is a significant behavior change that may affect mapping quality.
Incompatible field sets: emap_params adds fields like
matching_mode_t,area_oriented_mapping, andmap_multioutputthat map_params does not have.Before merging, verify:
- That
emap_paramsactually has averbosefield (or update CLI code)- That the cut limit change from 49 to 20 is intentional
- That all existing code compiles and tests pass with these changes
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp (5)
10651-10656: Docstring forvalidate_required_gatesis clear and aligned with behaviorThe description and
Throws:clause accurately document the validation step and themissing_required_gates_exception. No issues from a documentation perspective.
16977-16982: Exception documentation is consistent with validation semanticsThe new
missing_required_gates_exceptiondocstring clearly states when it is thrown and matches the wording used in the validator doc. The empty constructor doc entry is acceptable for generated bindings.
21942-21947: Technology mapping doc correctly reflects switch tomockturtle::emapThe updated description now explicitly mentions
mockturtle::emap, which matches the PR’s goal of changing the default mapper. This keeps the Python docs consistent with the new underlying implementation.
21984-21984: Inverter parameter docstring is precise and consistentThe description “1-input NOT gate (inverter).” is concise and matches the parameter name and expected semantics.
21994-21994: MUX parameter docstring is accurateThe “3-input MUX gate (ITE).” description is correct and consistent with typical terminology for this gate type and its use in mapping parameters.
cli/cmd/logic/include/map.hpp (1)
28-32: Docstring correctly reflects EMAP-based mappingThe updated comment to mention
mockturtle::emapaligns with the new default mapper and keeps the interface documentation accurate.docs/changelog.rst (1)
15-17: Changelog entry for EMAP switch is clearThe Algorithms/Changed note concisely documents the behavioral change to use
mockturtle::emapfor bothmapandtechnology_mapping, which matches the PR’s intent.cli/cmd/logic/src/map.cpp (2)
13-13: New includes match usage; just ensure fmt is linked for the CLI targetAdding
<fmt/format.h>and<type_traits>is consistent with the later use offmt::format,std::decay_t, andstd::is_same_v. Build should remain fine as long as the CLI target already links against fmt (or this translation unit is within a target that does).Also applies to: 18-18
76-79: Gate flag handling and preservation of control flags look soundStoring
ps.decayandps.mapper_params.verbosebefore applying--all2/--all3/--alland restoring them afterward avoids clobbering these controls when aggregate gate selections are used. Thegate_flagsarray combined with thestd::none_ofcheck correctly enforces that at least one gate-related or aggregate flag is set before mapping.No functional issues spotted here.
Also applies to: 96-98, 100-104
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
========================================
Coverage 98.16% 98.17%
========================================
Files 261 261
Lines 40105 40226 +121
Branches 2109 2118 +9
========================================
+ Hits 39369 39490 +121
Misses 736 736
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Signed-off-by: GitHub Actions <actions@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp (5)
10651-10656:validate_required_gatesdocstring is clear; consider qualifying the exception typeThe description of what is validated and that
missing_required_gates_exceptionis thrown is good. For extra clarity in the Python docs, you might name itfiction::missing_required_gates_exceptionto match other fully qualified types (if you follow that convention elsewhere), but this is optional.
14173-14173: Fill in the empty docstring forget_ntk_type_nameRight now the docstring is empty (
R"doc()doc"), sohelp()in Python won’t give users any indication of what this returns (e.g., whether it’s a human-readable name, enum-like string, etc.). Adding even a one-line summary like “Returns a human-readable type name for the given network/layout” would improve usability.
14189-14189: Fill in the empty docstring forget_sidb_lattice_nameSimilarly, this empty docstring doesn’t tell Python users what form the lattice name takes or how it’s derived. A short sentence (e.g., “Returns the canonical lattice name for a given SiDB layout”) would make the binding self-explanatory.
14209-14212: Add minimal descriptions forget_tech_cell_nameandget_tech_impl_nameBoth bindings currently have empty docstrings, which is a missed opportunity—these names are likely important concepts in the tech library. Adding brief docs (e.g., that one returns a human-readable cell name and the other the implementation name / variant) would help Python users navigate the API.
21953-21955:technology_mappingdoc correctly referencesmockturtle::emap; consider noting multi-output supportUpdating the docs to mention
mockturtle::emapaligns the Python help text with the implementation change. You might optionally add a short remark thatemapsupports multi-output cells (the core motivation of this PR) so Python users understand why this mapper is used by default. Since this is in a pyfiction binding header, it only affects Python docs, not the public C++ API. Based on learnings, ...test/algorithms/network_transformation/technology_mapping.cpp (1)
279-293: Exception tests for missing/complete gate sets match the new validation logicThe new tests:
- Verify that
technology_mappingthrowsmissing_required_gates_exceptionfor AIG/XAG/MIG when any of the required gates (INV, AND, XOR, MAJ) are missing, including “all missing” combinations.- Verify that no exception is thrown when the minimal required set is present for each network type.
- Keep a separate MIG name‑conservation test using
names_view, ensuring the mapping and name restoration still behave as expected.This test matrix maps cleanly onto the
validate_required_gates()implementation and should catch regressions if the required‑gate sets or the exception wiring change. The only (very minor) enhancement you might consider later is asserting on the exception message contents in at least one case to lock down thenetwork_typeandmissing_gatesformatting, but it’s not strictly necessary for this PR.Also applies to: 294-417
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hpp(1 hunks)bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp(8 hunks)include/fiction/algorithms/network_transformation/technology_mapping.hpp(8 hunks)include/fiction/types.hpp(2 hunks)test/algorithms/network_transformation/technology_mapping.cpp(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-25T10:31:01.287Z
Learnt from: marcelwa
Repo: cda-tum/fiction PR: 840
File: bindings/mnt/pyfiction/CMakeLists.txt:30-37
Timestamp: 2025-09-25T10:31:01.287Z
Learning: Headers in the pyfiction Python bindings target (bindings/mnt/pyfiction/) are private implementation details for the Python bindings and should not be made public or exported as part of the C++ API.
Applied to files:
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp
📚 Learning: 2025-10-13T11:04:24.736Z
Learnt from: marcelwa
Repo: cda-tum/fiction PR: 854
File: cli/cmd/logic/src/abc.cpp:78-88
Timestamp: 2025-10-13T11:04:24.736Z
Learning: The `mockturtle::write_aiger` function in the mockturtle library supports writing all kinds of logic networks (AIG, XAG, MIG, TEC), not just AIG networks. No explicit conversion to AIG is required before calling this function.
Applied to files:
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpptest/algorithms/network_transformation/technology_mapping.cppinclude/fiction/algorithms/network_transformation/technology_mapping.hppinclude/fiction/types.hpp
📚 Learning: 2025-10-13T14:43:13.479Z
Learnt from: hibenj
Repo: cda-tum/fiction PR: 852
File: include/fiction/algorithms/physical_design/orthogonal_planar.hpp:825-826
Timestamp: 2025-10-13T14:43:13.479Z
Learning: In the fiction library, logic networks are required to have at least one primary input (PI). The `orthogonal_planar` algorithm in `include/fiction/algorithms/physical_design/orthogonal_planar.hpp` enforces this invariant with an assertion rather than handling the zero-PI case.
Applied to files:
include/fiction/algorithms/network_transformation/technology_mapping.hppinclude/fiction/types.hpp
📚 Learning: 2025-09-10T16:31:45.364Z
Learnt from: marcelwa
Repo: cda-tum/fiction PR: 828
File: include/fiction/technology/area.hpp:0-0
Timestamp: 2025-09-10T16:31:45.364Z
Learning: In include/fiction/technology/area.hpp, both fiction::area overloads should accept parameters as `const area_params<technology<Lyt>>& ps = {}` to allow binding the default temporary and ensure const-correctness. Docs in docs/technology/properties.rst should mirror these signatures.
Applied to files:
include/fiction/types.hpp
🧬 Code graph analysis (1)
test/algorithms/network_transformation/technology_mapping.cpp (2)
bindings/mnt/pyfiction/include/pyfiction/algorithms/network_transformation/technology_mapping.hpp (4)
technology_mapping(23-30)technology_mapping(23-23)technology_mapping(34-96)technology_mapping(34-34)include/fiction/algorithms/network_transformation/technology_mapping.hpp (4)
technology_mapping(638-639)all_supported_standard_functions(249-249)missing_required_gates_exception(37-40)missing_required_gates_exception(37-37)
🔇 Additional comments (7)
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp (3)
16985-16990:missing_required_gates_exceptiondocumentation is well-scopedThe new exception docstring clearly states when it is thrown and ties it to missing required gates for the base network type, which matches the surrounding mapping changes. The separate (empty) ctor docstring is fine given that the default constructor has no extra semantics to explain.
21992-21993: Docstring fortechnology_mapping_params_invlooks consistentThe description “1-input NOT gate (inverter).” matches the naming and style of the other gate parameter docstrings (e.g.,
gt2,le2) and should be clear to users.
22002-22003: Docstring fortechnology_mapping_params_muxis clear and consistent“3-input MUX gate (ITE).” accurately and succinctly describes the gate and matches the style of neighboring parameter docs, so this looks good.
test/algorithms/network_transformation/technology_mapping.cpp (2)
6-7: Test scaffolding improvements are appropriateAdding
<catch2/catch_test_macros.hpp>and moving the helper functions into an anonymous namespace are both sensible: the former is required for the newTEST_CASEs, and the latter avoids polluting the global namespace from test helpers. No issues here.Also applies to: 21-23, 226-227
262-277: Complex all‑function mapping test aligns well with EMAP behaviorThe new “Complex 3‑input” MIG‑only test and the “Complex all function” test using
all_supported_standard_functions()give good coverage of EMAP with richer gate libraries across AIG/XAG/MIG/XMG:
- They assert
!mapping_errorand logical equivalence viacheck_eq.all_supported_standard_functions()correctly exercises both 2‑ and 3‑input gate cases, matching the EMAP use case for multi‑output cells.This looks like a solid regression test for the mapper switch.
include/fiction/types.hpp (1)
57-73: Original concern about raw network type usage is unfounded—refactor is working as intendedVerification shows the codebase consistently uses the aliased network types (
aig_nt,xag_nt,mig_nt,tec_nt), not raw mockturtle types. Thelogic_network_tvariant stores only aliased pointer types (aig_ptr,xag_ptr,mig_ptr,tec_ptr), all instantiations create aliased types, and all call sites pass the extracted aliased types tontk_type_name. The hypothetical behavioral concern about silent failures with raw network types does not occur in practice—there are no raw network types being passed anywhere. The refactor is clean and correct.Likely an incorrect or invalid review comment.
include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
14-18: EMAP migration verified and correctThe
mockturtle::emapfunction signature isemap(Ntk const &ntk, tech_library<NInputs, Configuration> const &library, emap_params const &ps = {}, emap_stats *pst = nullptr). This matches the parameter ordering, types, and defaults reflected in your code: the includes,emap_params,emap_stats, and the call inperform_mappingare all correctly wired to the v0.4 API. No issues with the EMAP migration.
Signed-off-by: GitHub Actions <actions@github.com>
Signed-off-by: GitHub Actions <actions@github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp (1)
14173-14174: Consider adding documentation for helper functions.Several new helper functions have empty docstrings:
get_ntk_type_name(network type name resolution)get_sidb_lattice_name(lattice name resolution)get_tech_cell_name(technology cell naming)get_tech_impl_name(technology implementation naming)Since this is an auto-generated file, add docstrings to the corresponding C++ source code if these functions are part of the public Python API.
Note: This aligns with the unchecked "added documentation" item in the PR checklist.
Also applies to: 14189-14190, 14209-14212
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp(9 hunks)cli/cmd/logic/src/map.cpp(5 hunks)include/fiction/algorithms/network_transformation/technology_mapping.hpp(11 hunks)include/fiction/technology/technology_mapping_library.hpp(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-25T10:31:01.287Z
Learnt from: marcelwa
Repo: cda-tum/fiction PR: 840
File: bindings/mnt/pyfiction/CMakeLists.txt:30-37
Timestamp: 2025-09-25T10:31:01.287Z
Learning: Headers in the pyfiction Python bindings target (bindings/mnt/pyfiction/) are private implementation details for the Python bindings and should not be made public or exported as part of the C++ API.
Applied to files:
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp
📚 Learning: 2025-10-13T11:04:24.736Z
Learnt from: marcelwa
Repo: cda-tum/fiction PR: 854
File: cli/cmd/logic/src/abc.cpp:78-88
Timestamp: 2025-10-13T11:04:24.736Z
Learning: The `mockturtle::write_aiger` function in the mockturtle library supports writing all kinds of logic networks (AIG, XAG, MIG, TEC), not just AIG networks. No explicit conversion to AIG is required before calling this function.
Applied to files:
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hppinclude/fiction/algorithms/network_transformation/technology_mapping.hpp
📚 Learning: 2025-09-08T13:33:29.693Z
Learnt from: Drewniok
Repo: cda-tum/fiction PR: 826
File: experiments/figure_of_merit_analysis/fom_analysis_2_input_1_output.cpp:5-6
Timestamp: 2025-09-08T13:33:29.693Z
Learning: In the fiction codebase, the preprocessor guard pattern `#if (FICTION_ALGLIB_ENABLED)` is preferred over `#if defined(FICTION_ALGLIB_ENABLED) && FICTION_ALGLIB_ENABLED` for ALGLIB feature detection. The current approach is sufficient for their build system configuration.
Applied to files:
include/fiction/algorithms/network_transformation/technology_mapping.hpp
📚 Learning: 2025-09-10T16:31:45.364Z
Learnt from: marcelwa
Repo: cda-tum/fiction PR: 828
File: include/fiction/technology/area.hpp:0-0
Timestamp: 2025-09-10T16:31:45.364Z
Learning: In include/fiction/technology/area.hpp, both fiction::area overloads should accept parameters as `const area_params<technology<Lyt>>& ps = {}` to allow binding the default temporary and ensure const-correctness. Docs in docs/technology/properties.rst should mirror these signatures.
Applied to files:
include/fiction/algorithms/network_transformation/technology_mapping.hpp
📚 Learning: 2025-10-13T14:43:13.479Z
Learnt from: hibenj
Repo: cda-tum/fiction PR: 852
File: include/fiction/algorithms/physical_design/orthogonal_planar.hpp:825-826
Timestamp: 2025-10-13T14:43:13.479Z
Learning: In the fiction library, logic networks are required to have at least one primary input (PI). The `orthogonal_planar` algorithm in `include/fiction/algorithms/physical_design/orthogonal_planar.hpp` enforces this invariant with an assertion rather than handling the zero-PI case.
Applied to files:
include/fiction/algorithms/network_transformation/technology_mapping.hppinclude/fiction/technology/technology_mapping_library.hpp
🧬 Code graph analysis (1)
cli/cmd/logic/src/map.cpp (1)
include/fiction/algorithms/network_transformation/technology_mapping.hpp (1)
technology_mapping(642-643)
🔇 Additional comments (25)
bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp (4)
10651-10656: LGTM!The docstring accurately documents the validation function and its exception behavior, aligning with the new technology mapping validation feature.
16985-16990: LGTM!The exception class is properly documented with a clear description of when it's thrown. The empty constructor docstring is acceptable for standard exception constructors.
21950-21976: LGTM!The documentation accurately reflects the change from
mockturtle::maptomockturtle::emapand provides comprehensive information about:
- The new validation behavior
- The exception that can be thrown
- Required gates for different network types (AIG, XAG, MIG)
This aligns perfectly with the PR objectives.
21998-22000: LGTM!The parameter docstrings provide clear, concise descriptions of the gate types. The technical terminology (half-adder, inverter, ITE) is accurate and appropriate for the target audience.
Also applies to: 22010-22010
cli/cmd/logic/src/map.cpp (6)
13-18: LGTM! Appropriate includes for new functionality.The
fmt/format.hinclude supports the new formatted warning/error messages, andtype_traitsis needed forstd::decay_tandstd::is_same_vused in the mapping lambda.
37-37: LGTM! Half-adder gate flag added correctly.The new
--haflag aligns with the expanded gate support in the EMAP-based technology mapping.
78-99: LGTM! Simplified control flag handling.The removal of
logic_sharingflag handling is consistent with the switch to EMAP, which does not use this parameter. The save/restore pattern fordecayandverboseflags is correctly preserved.
101-105: LGTM! Gate flags array updated to include new gates.The array correctly includes
haand maintains proper alignment with the available gate flags.
116-123: Good defensive warning for remapping scenarios.The warning when attempting to map an already-mapped
tec_ntnetwork helps users understand potential issues. However, note that the warning will always be printed fortec_ntnetworks even if remapping succeeds without errors.
127-142: Exception handling correctly integrated.The try/catch block properly catches
fiction::missing_required_gates_exceptionand prints the exception message. The early return onmapping_errorprevents adding an invalid network to the store.One observation: when
mapping_erroris set, the network is not added to the store, which is correct. However, whenmissing_required_gates_exceptionis caught, the exception is printed but flow continues normally (no store extension, which is also correct).include/fiction/algorithms/network_transformation/technology_mapping.hpp (11)
13-25: LGTM! Includes properly updated for EMAP integration.The explicit
#include <fmt/format.h>addresses the previous review comment about fragile header ordering. The mockturtle network headers are appropriately added to support thestd::is_same_vchecks invalidate_required_gates().
31-41: Well-designed exception class for gate validation failures.The exception inherits from
std::logic_errorappropriately since missing required gates represents a programming/configuration error. The formatted message clearly indicates both the network type and missing gates.
104-107: LGTM! Half-adder gate parameter added.The
haboolean flag is correctly added to the 2-input functions section with appropriate documentation.
337-342: Consider passingtechnology_mapping_paramsby const reference.The static analysis hint from a past review noted that
technology_mapping_paramsis 136 bytes. The constructor already takes it by const reference (const technology_mapping_params& ps), which is appropriate.
344-348: Good addition of[[nodiscard]]andconstqualifiers.The
[[nodiscard]]attribute ensures the mapped result isn't accidentally discarded, and makingrun()const-qualified is appropriate since it doesn't modify the impl object's state.
358-359: LGTM! Half-adder gate included in 2-input mapping path.The
params.hacheck is correctly added to the condition that triggers 2-input gate mapping.
384-445: Well-structured gate validation with comprehensive coverage.The
validate_required_gates()function correctly validates:
- AIG networks require INV and AND
- XAG networks require INV, AND, and XOR
- MIG networks require INV and MAJ
The use of
Ntk::base_typefor type detection is the established pattern in mockturtle. Networks not matching these base types (e.g.,tec_ntbeing remapped) will pass validation without checks, which aligns with the CLI warning for remapping scenarios.
504-507: LGTM! Half-adder gate library integration.The conditional addition of
GATE_HAto the library stream follows the same pattern as other 2-input gates.
611-611: Core change: Switch tomockturtle::emap.This is the central change of the PR—replacing
mockturtle::mapwithmockturtle::emap. The EMAP algorithm supports multi-output cells (like the new half-adder), which is the stated motivation for this change.
628-643: Documentation correctly updated for new exception behavior.The
@throwsdocumentation clearly specifies the exception type and conditions under which it's thrown, which is good practice for API documentation.
48-48: Let me search for mockturtle API documentation to verify the compatibility:
mockturtle emap_params documentation and APIinclude/fiction/technology/technology_mapping_library.hpp (4)
18-25: LGTM! Type declarations simplified toauto.Changing from
inline constexpr const char*toinline constexpr autois a stylistic improvement. The deduced type remainsconst char*, maintaining the same semantics while reducing verbosity.
30-42: LGTM! 2-ary gate declarations updated consistently.All 2-input gate constants follow the same pattern with the
autotype specifier.
44-48: New half-adder gate enables multi-output cell support.The
GATE_HAdefinition correctly specifies both outputs of a half-adder:
C=a*b(carry output)S=!a*b+a*!b(sum output, equivalent to XOR)This is the key addition that leverages EMAP's multi-output cell capability. The genlib format with two GATE lines for the same cell name is the standard way to define multi-output gates.
54-99: LGTM! 3-ary gates and decay variants updated consistently.All 3-input gate definitions and their decay variants maintain correct genlib syntax. The multi-line string concatenation for decay variants is clean and readable.
# Conflicts: # .github/workflows/clang-tidy-review.yml # bindings/mnt/pyfiction/CMakeLists.txt # docs/changelog.rst
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@bindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpp`:
- Around line 14189-14190: Add a proper Doxygen comment block to the C++
declaration for get_sidb_lattice_name (e.g., /** `@brief` ... `@return` ... */)
describing what the function does and its return value, then regenerate the
mkdoc output so __doc_fiction_get_sidb_lattice_name is populated; update the
declaration for get_sidb_lattice_name (and any overloads) with `@brief` and
`@return` tags following the project's Doxygen style and run the mkdoc generation
step to refresh pybind11_mkdoc_docstrings.hpp.
- Around line 14173-14174: The docstring for get_ntk_type_name is empty (static
const char *__doc_fiction_get_ntk_type_name), so the Python binding exposes no
help text; fix by adding a Doxygen comment block to the C++ declaration of
get_ntk_type_name (use /** `@brief` ... `@return` ... */ with appropriate
description of what the function returns and any parameters if present) in the
source header where get_ntk_type_name is declared, then regenerate the mkdoc
output so the generated __doc_fiction_get_ntk_type_name contains the new text;
ensure you follow the project guideline to use modern Doxygen tags (`@brief`,
`@return`, etc.) and apply it to the exact symbol get_ntk_type_name so bindings
pick it up.
- Around line 14209-14212: Add Doxygen /** ... */ comments to the underlying C++
declarations for get_tech_cell_name and get_tech_impl_name (not just the
generated mkdoc strings) including `@brief` describing what each function returns
and an `@return` describing the return value; after updating the function
declarations, regenerate the mkdoc output so __doc_fiction_get_tech_cell_name
and __doc_fiction_get_tech_impl_name are populated. Ensure the Doxygen blocks
use modern commands (e.g., `@brief`, `@return`, `@param` if any) and are placed
immediately above the function signatures for get_tech_cell_name and
get_tech_impl_name.
- Around line 16986-16991: The docstring for the constructor of
missing_required_gates_exception is empty; add a Doxygen /** ... */ comment to
the C++ constructor declaration/definition for class
missing_required_gates_exception (include an `@brief` describing the exception and
`@throws` or `@param` tags as appropriate) in the original C++ source where the
constructor is defined, then regenerate the mkdoc output so
pybind11_mkdoc_docstrings.hpp contains the populated
__doc_fiction_missing_required_gates_exception_missing_required_gates_exception
entry; ensure the Doxygen uses modern commands (`@brief`, `@throws`) per guidelines.
…pyfiction/documentation.hpp' file not found"
…e ReadTheDocs build command to not use IPO; default to Release mode for wheels builds
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@bindings/mnt/pyfiction/CMakeLists.txt`:
- Around line 11-18: PYFICTION_LTO_OPTIONS may retain a value from parent scope
when FICTION_ENABLE_IPO is OFF, causing unwanted LTO flags to be passed to
pybind11_add_module; update the CMake logic around FICTION_ENABLE_IPO to
explicitly unset or clear the PYFICTION_LTO_OPTIONS variable when
FICTION_ENABLE_IPO is false so only when FICTION_ENABLE_IPO is true you set
PYFICTION_LTO_OPTIONS to THIN_LTO, and ensure pybind11_add_module(pyfiction ...)
only sees PYFICTION_LTO_OPTIONS when it was explicitly set by this CMakeLists
(reference FICTION_ENABLE_IPO, PYFICTION_LTO_OPTIONS, and pybind11_add_module).
…ent build inactivity
…ntainability; force unbuffered output
…ing CMake arguments for improved build performance
…ons instead of replacing them
…for improved memory management
There was a problem hiding this comment.
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 `@pyproject.toml`:
- Around line 62-63: The global CMake default is set to cmake.build-type =
"Release" but the ReadTheDocs override still injects -O0; update the
ReadTheDocs-specific configuration so it explicitly sets cmake.build-type =
"Debug" (instead of relying on individual CFLAGS/LDFLAGS) to consolidate
optimization settings, remove or stop overriding with -O0 in that override
block, and ensure the ReadTheDocs override and the global cmake.build-type
setting are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ab655bb-e316-49de-9b75-feaaeeb701f6
📒 Files selected for processing (3)
.github/workflows/clang-tidy-review.ymlbindings/mnt/pyfiction/include/pyfiction/pybind11_mkdoc_docstrings.hpppyproject.toml
| # Set default build type to Release | ||
| cmake.build-type = "Release" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "READTHEDOCS|mold|CMAKE_CXX_FLAGS|cmake.build-type|-fuse-ld" pyproject.toml
rg -n "apt(-get)? .*mold|brew .*mold|choco .*mold|dnf .*mold|pacman .*mold" -g "*.yml" -g "*.yaml" -g "*.toml" -g "*.md"
fd -HI '^\.readthedocs\.ya?ml$' -x sed -n '1,200p' {} || trueRepository: cda-tum/fiction
Length of output: 310
🏁 Script executed:
find . -name ".readthedocs*" -type f | head -20Repository: cda-tum/fiction
Length of output: 77
🏁 Script executed:
sed -n '115,135p' pyproject.tomlRepository: cda-tum/fiction
Length of output: 628
🏁 Script executed:
rg -l "mold" --type py --type sh --type json --type toml --type yamlRepository: cda-tum/fiction
Length of output: 90
🏁 Script executed:
cat -n .readthedocs.ymlRepository: cda-tum/fiction
Length of output: 1447
Consolidate conflicting optimization levels in ReadTheDocs override.
The global cmake.build-type = "Release" on line 63 conflicts with the -O0 flag in the ReadTheDocs override (line 125). The ReadTheDocs override should explicitly set cmake.build-type = "Debug" instead of relying on per-flag overrides.
Note: mold is properly provisioned in .readthedocs.yml (line 9), so the linker flags are not a fragility concern. However, consolidating the build type setting is cleaner and prevents potential configuration confusion.
Suggested config adjustment
[[tool.scikit-build.overrides]]
if.env.READTHEDOCS = "True"
inherit.cmake.define = "append"
cmake.define.FICTION_ENABLE_IPO = "OFF"
+cmake.build-type = "Debug"
cmake.args = [
"-DCMAKE_VERBOSE_MAKEFILE=ON",
- "-DCMAKE_CXX_FLAGS=-O0",
- "-DCMAKE_MODULE_LINKER_FLAGS=-fuse-ld=mold",
- "-DCMAKE_SHARED_LINKER_FLAGS=-fuse-ld=mold",
- "-DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=mold"
]Also applies to: 123-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 62 - 63, The global CMake default is set to
cmake.build-type = "Release" but the ReadTheDocs override still injects -O0;
update the ReadTheDocs-specific configuration so it explicitly sets
cmake.build-type = "Debug" (instead of relying on individual CFLAGS/LDFLAGS) to
consolidate optimization settings, remove or stop overriding with -O0 in that
override block, and ensure the ReadTheDocs override and the global
cmake.build-type setting are consistent.
Fixes discovered during review of the mockturtle::map -> emap switch: - technology_mapping_library.hpp: the two GATE_HA lines shared the same genlib name "ha", so mockturtle classified them as a single multi-output cell and (with default library settings) excluded them from the single-output library, making `ps.ha`/`--ha` a no-op that could only ever produce a mapping error. Give the sum/carry gates distinct names (ha_carry/ha_sum) so they load as two usable single-output gates. - Add C++ tests exercising the half-adder path: mapping a technology_network half-adder/full-adder using only the ha gates (verifying both carry and sum are used and the result is equivalent), plus ha combined with the standard 2-input library. - Expose the `ha` parameter in the pyfiction technology_mapping_params binding and add a Python test. - Restore the technology mapping test matrix that had been narrowed to mig-only: AIG/MIG/XMG for the AOIM and name-conservation cases and MIG/XMG for the 3-input case. The remaining exclusions (XAG everywhere, AIG in the 3-input case) are intentional: the new validate_required_gates() correctly rejects those combinations because the chosen helper params omit XOR/AND. - Remove the now-dead map_and_check_all_func / map_and_check_all_standard_func test helpers. - changelog: add entries for half-adder support, the new missing_required_gates_exception, and the breaking emap_params/emap_stats and CLI behavior changes; move the already-released "macOS 13 / x86_64" removal note back under v0.6.12. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AABcKjPw3jsm6kboRDrMdi
Signed-off-by: Marcel Walter <marcel.walter@tum.de>
3 participants
Description
This PR switches the default technology mapper in fiction from
mockturtle::maptomockturtle::emapin both thefiction::technology_mappingfunction and themapCLI command. The change is made becausemockturtle::emapsupports multi-output cells, whereasmockturtle::mapdoes not.Checklist:
Summary by CodeRabbit
New Features
Changes
Breaking Changes
Documentation
Tests
Chores