gpl/mbff: preserve hierarchy in cluster_flops

[openroad-ci]

Summary

Problem

cluster_flops destroyed module hierarchy. Original flip-flops live
inside hierarchical sub-modules (dbModule), but MBFF created the new
tray instance directly on the top dbBlock and destroyed the originals.
After clustering, sub-modules became empty shells and trays sat at the
top, breaking write_verilog, hierarchical reports, scan stitching,
power-domain assignment, and any downstream consumer that relies on
module ownership.

Root cause: dbInst::create(block_, master, name) in
MBFF::ModifyPinConnections ignored the parent dbModule. Compounding
this, MBFF::SeparateFlops partitioned candidate flops only by clock
net and Mask, so flops from different modules could be merged into a
single tray with no valid home module.

Fix

Two surgical changes in src/gpl/src/mbff.cpp:

1. SeparateFlops: partition by (dbModule*, Mask) instead of Mask
   alone. Flops in different modules can no longer share a tray.

2. ModifyPinConnections: pass the cluster's parent module to the
   dbInst::create(block, master, name, physical_only, parent_module)
   overload, so the new tray lands in the same module as the flops it
   replaces. Same pattern already used by cts/src/TritonCTS.cpp.

Mask itself is untouched, so library-cell tables keyed by Mask
(best_master_, pin_mappings_, slot_to_tray_x_, etc.) are
unaffected.

Behavior

• Flat designs: identical results (existing mbff_orig_name regression
  unchanged).
• Hierarchical designs: trays are created inside the correct submodule.
  Cross-module clustering is no longer attempted; this is a small QoR
  trade for correctness. If a future -flatten_hierarchy flag is
  desired, it can be added on top of this partitioning.

Test

New regression src/gpl/test/mbff_hier.{v,tcl,ok} registered in
CMakeLists.txt and BUILD:

• 4 flip-flops split across two child modules (sub_a, sub_b).
• Drives cluster_flops and prints per-module instance counts before
  and after.

Pre-fix output:
AFTER: top={_tray_size4_7} sub_a={} sub_b={}
Post-fix golden:
AFTER: top={} sub_a={_tray_size2_X} sub_b={_tray_size2_Y}

Type of Change

• Bug fix

Impact

Fix hier design' cluster flops

Verification

• I have verified that the local build succeeds (./etc/Build.sh).
• I have run the relevant tests and they pass.
• My code follows the repository's formatting guidelines.
• I have included tests to prevent regressions.
• I have signed my commits (DCO).

Related Issues

https://github.com/The-OpenROAD-Project-private/OpenROAD/issues/3392

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[gemini-code-assist]

Code Review

This pull request introduces hierarchical awareness to Multi-Bit Flip-Flop (MBFF) clustering, ensuring that flops are only grouped within their respective hierarchical modules. This is achieved by partitioning flops based on both their parent module and mask, and ensuring new trays are created within the correct module context. A new regression test, mbff_hier, has been added to verify this behavior. Review feedback suggests using module IDs instead of pointers as map keys to avoid non-deterministic iteration order and ensure bit-level reproducibility.

[gemini-code-assist]

Using dbModule* as part of a std::map key can lead to non-deterministic behavior across different runs because the iteration order depends on memory addresses. To ensure bit-level reproducibility, consider using a deterministic key such as dbModule::getId() or providing a custom comparator (e.g., std::lessodb::dbModule*) as iteration over a std::map with pointer keys is deterministic if a custom comparator is defined for the pointer type.

    std::map<std::pair<uint, Mask>, std::vector<Flop>> flops_by_mod_mask;

References

1. Iteration over a std::map with pointer keys is deterministic if a custom comparator is defined for the pointer type.

[gemini-code-assist]

If the map key is changed to use the module ID for determinism, update the insertion logic accordingly. Note that you may also need to store the dbModule* pointer if it's required for later use (e.g., in the debug print), or look it up via the ID.

      flops_by_mod_mask[{insts_[idx]->getModule()->getId(), vec_mask}].push_back(
          flops_[idx]);

[jhkim-pii]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Bravo.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jhkim-pii]

This is a flat flow. link_design -hier mbff_hier should be used to enable the hierarchical flow.

[dsengupta0628]

Actually even with -hier I don't see crash. Same wrong-hierarchy output (tray in top, submods empty). Bug is silent corruption, not segfault.

[dsengupta0628]

Do you have a testcase to show the crash by the way?

[jhkim-pii]

This is the first issue reported.
https://github.com/The-OpenROAD-Project-private/OpenROAD-flow-scripts/issues/1628

[dsengupta0628]

I created this issue that doesn't crash but doesn't give correct results either: #10339

[jhkim-pii]

An important part is missing. There is no dbModNet connection logic, so the resulting ODB will still cause crash when hier flow is enabled.

The following logic should be added.

• Capture dbModNet* before disconnecting each original iterm and reconnect the replacement iterm with both flat and hierarchical nets, preferably through dbNetwork::connectPin or the dual dbITerm::connect(dbNet*, dbModNet*).

[jhkim-pii]

This is just a suggestion.
When hier flow is enabled, ff_a1_ff_a2 looks better than _tray_size2_7.

In flat flow, such name concatenation is not easy because the FF hierarchies are different and it will produce too long MBFF name.
But in hier flow, the MBFF name will not be very long.

[QuantamHD]

This seems like rather negative regression. Is there a way to support merging flops in different modules? It feels like that would in general yield better PPA

[dsengupta0628]

#10339

[QuantamHD]

I guess in general the downside would be we would have to port punch a lot unless there's some way I'm not thinking about.

module top_level (
    input wire clk
);

    // Wires connecting the sub-modules to the shared flop
    wire [3:0] mod_a_d, mod_a_q;
    wire [3:0] mod_b_d, mod_b_q;
    
    // Instantiate Module A
    module_a inst_a (
        .clk(clk),
        .data_out(mod_a_d), // Goes to D-pin of the shared flop
        .data_in(mod_a_q)   // Comes from Q-pin of the shared flop
    );

    // Instantiate Module B
    module_b inst_b (
        .clk(clk),
        .data_out(mod_b_d),
        .data_in(mod_b_q)
    );

    // Instantiate the shared 8-bit flip-flop
    // Using concatenation {b, a} to merge the two 4-bit inputs
    flop_8bit shared_ff_inst (
        .clk(clk),
        .d({mod_b_d, mod_a_d}), 
        .q({mod_b_q, mod_a_q})  
    );

endmodule

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[dsengupta0628]

This seems like rather negative regression. Is there a way to support merging flops in different modules? It feels like that would in general yield better PPA

Yes this does reduce banking ratio compared to flat leaving some QoR on the table as we are only clustering in current hierarchy in this change. However, currently the MBFF clustering is wrong in hier designs (I was told crash- can't reproduce, I see wrong results though), so I put this as an interim fix before a more elaborate change is applied that would collapse hierarchy, do clustering and then rebuild hierarchy. That will take some more thinking.

[jhkim-pii]

If the massive port punching is ok, it is doable.
But I wonder if the hierarchy preserving is still required or not because many port punching makes the hierarchy extremely messy.
I am just curious about this.

• What is the value of maintaining messy hierarchy?
• If very messy hierarchy is not a concern, why is the flattened netlist (no hierarchy) not allowed?

There can be a hybrid approach. Preserving hierarchies up to a certain level and flattens deep hierarchies. But current OpenROAD does not support this yet.

[jhkim-pii]

Ah, operator swap could be the reason of preserving messy hierarchy because ALU swap cannot be done on flat netlist.