[tnguy19]

Summary

• Integrates openSTA into the RAM module to display timing and power after RAM generation

• Timing report will be run first to determine the max delay, then this value will be used as the clock period to determine the power of the RAM.

Type of Change

• New feature

Impact

After generating a RAM, user can now see timing and power report numbers being reported automatically

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

#9392

@rovinski

[gemini-code-assist]

Code Review

This pull request integrates the Static Timing Analysis (STA) engine into the RAM generator, adding methods to retrieve and report the worst setup slack and total power of the generated RAM. The reviewer recommends adding defensive checks to prevent segmentation faults if these reporting methods are called before a RAM block is generated, as well as handling unconstrained timing values in the Tcl interface to avoid formatting errors.

[rovinski]

user can now use report_ram_timing to query for worst setup slack number, report_ram_power to query for RAM power, and report_ram_summary to get both numbers

There is no need for a user to manually call these functions. Make them internal functions which are called at the end of the ram generation automatically.

[rovinski]

For now, you should assume that no timing constraints have been applied to the RAM. Report the unconstrained worst slack for both hold and setup, as that represents the cycle time of the RAM.

I don't know what STA gives you for the power number if there aren't any activity factors propagated.

[tnguy19]

I have made the updates to the code, ready for review @rovinski

[rovinski]

My comment about the power meant that I don't know myself what STA does when you estimate power, but there haven't been any activity factors applied. It might generate them, or it might just give back bogus results. It is worth finding out.

[rovinski]

@gemini-code-assist /review

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[maliberty]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3fac55cf1a

ℹ️ 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".

[chatgpt-codex-connector]

Move reports after final RAM generation

In the generate_ram Tcl flow this call runs inside generate_ram_netlist, but ram.tcl then continues with ord::design_created, PDN insertion, pin placement, filler insertion, and routing. For normal generate_ram users the newly logged timing/power therefore describes the pre-PDN/pre-filler netlist; cells inserted by ram_filler are omitted from the reported total power, so the number is stale for the final RAM macro.

Useful? React with 👍 / 👎.

[tnguy19]

Updated in 7f6271e

[chatgpt-codex-connector]

Filter zero-length paths before logging minimum delay

With the unconstrained min-path query left unfiltered, designs with primary/clock startpoints at zero arrival can be selected as the “shortest” path; the new goldens all report RAM minimum path delay: 0.000 ns, which is not a useful RAM min/cycle delay. Please constrain/filter the returned path to real data paths before logging it, otherwise users get a silently misleading minimum-delay report.

Useful? React with 👍 / 👎.

[tnguy19]

Did some investigation and found that because no create_clock is defined in these automated RAM test the fastest min path appear to always be through the latch/flip-flop clock pins which will report a value of zero second which is misleading.

I added in a check to restrict the pin search to only the functional endpoints (flip-flop/latch D pins and primary Q outputs) since this is more accurate of how the RAMs will perform. This should also works with any RAM sizes and configurations instead of trying to define a specific number of pins that we want to restrict the test to.

Update is made in c121752

[rovinski]

I think the real solution is just that clock should be excluded and everything else should be included.

You may also need to set clocks to propagated? I'm not sure if that affects the datapath timing.

[rovinski]

I'm pausing review until the paper is submitted.

[maliberty]

@rovinski ready to move forward?

[rovinski]

Targeting paper submission within the next week or two.

[maliberty]

Undraft when ready to proceed

[tnguy19]

@rovinski The code is updated to first run the timing report and find the max delay, then that max delay is set as the clock period for the total power report. The log will shows as follows for the case of a 8x8 RAM :

[INFO RAM-0044] RAM maximum path delay: 1.114 ns
[INFO RAM-0045] RAM minimum path delay: 0.399 ns
[INFO RAM-0047] Using maximum path delay of 1.114 ns as the clock period for power estimation
[INFO RAM-0046] RAM estimated power: 848681.312 nW

The PR descirption is also updated to reflect this latest change. Ready for review

[rovinski]

Can you change this to be RAM estimated power at max frequency:

And then also add a line for RAM estimated energy efficiency: {} {}

Where the energy efficiency should be calculated as (power/frequency)/(bits per word) with the units as {}J/bit, and {} is probably on the order of pJ or fJ.

[tnguy19]

@rovinski ready for review

[rovinski]

@gemini-code-assist /review

[gemini-code-assist]

Code Review

This pull request integrates static timing analysis (STA) into the RAM generator, enabling it to report maximum and minimum path delays, estimate power at maximum frequency, and calculate energy efficiency. Feedback on the changes highlights multiple critical memory leaks in the newly implemented RamGen::reportTiming() function, where heap-allocated sta::PinSet, exception objects, and path ends are not properly cleaned up. Consolidating the redundant exception allocations and ensuring proper deletion of these objects is recommended.

[gemini-code-assist]

There are multiple memory leaks in RamGen::reportTiming():\n\n1. sta::PinSet Leaks: collect_primary_inputs and collect_endpoints allocate sta::PinSet on the heap using new but they are never deleted.\n2. sta::ExceptionFrom and sta::ExceptionTo Leaks: sta_->makeExceptionFrom and sta_->makeExceptionTo allocate exception objects on the heap which must be deleted by the caller.\n3. sta::PathEnd Leaks: sta_->findPathEnds allocates PathEnd objects inside the returned PathEndSeq sequence, which must be deleted to avoid leaking memory.\n\nAdditionally, setup_from/hold_from and setup_to/hold_to are identical and can be consolidated into a single exception_from and exception_to to avoid redundant allocations.\n\nWe can resolve all of these issues by:\n- Allocating sta::PinSet on the stack and passing their addresses.\n- Reusing the exception objects for both setup and hold analysis.\n- Explicitly deleting the allocated exceptions and path ends at the end of the function.

double RamGen::reportTiming()\n{\n  network_->setBlock(block_);\n  sta_->updateTiming(false);\n\n  sta::SceneSeq scenes = sta_->makeSceneSeq(sta_->cmdScene());\n  sta::StringSeq group_names;\n\n  auto collect_primary_inputs = [&](sta::PinSet& pins) {\n    for (auto* bterm : data_inputs_) {\n      pins.insert(network_->dbToSta(bterm));\n    }\n    for (auto& port_addrs : addr_inputs_) {\n      for (auto* bterm : port_addrs) {\n        pins.insert(network_->dbToSta(bterm));\n      }\n    }\n    for (auto* bterm : write_enable_) {\n      pins.insert(network_->dbToSta(bterm));\n    }\n  };\n\n  auto collect_endpoints = [&](sta::PinSet& pins) {\n    for (auto* inst : block_->getInsts()) {\n      for (auto* iterm : inst->getITerms()) {\n        auto* pin = network_->dbToSta(iterm);\n        if (!pin || isClockPin(pin, network_)) {\n          continue;\n        }\n        auto* lp = network_->libertyPort(pin);\n        if (lp && (lp->direction()->isAnyOutput() || lp->isClockGateEnable())) {\n          pins.insert(pin);\n        }\n      }\n    }\n    for (auto* bterm : block_->getBTerms()) {\n      auto* pin = network_->dbToSta(bterm);\n      if (pin && !isClockPin(pin, network_)\n          && network_->direction(pin)->isOutput()) {\n        pins.insert(pin);\n      }\n    }\n  };\n\n  sta::PinSet primary_inputs(network_);\n  collect_primary_inputs(primary_inputs);\n\n  sta::PinSet endpoints(network_);\n  collect_endpoints(endpoints);\n\n  // Find worst unconstrained setup path delay/longest path\n  sta::ExceptionFrom* exception_from\n      = sta_->makeExceptionFrom(&primary_inputs,\n                                nullptr,\n                                nullptr,\n                                sta::RiseFallBoth::riseFall(),\n                                sta_->cmdSdc());\n\n  sta::ExceptionTo* exception_to\n      = sta_->makeExceptionTo(&endpoints,\n                              nullptr,\n                              nullptr,\n                              sta::RiseFallBoth::riseFall(),\n                              sta::RiseFallBoth::riseFall(),\n                              sta_->cmdSdc());\n\n  sta::PathEndSeq setup_ends\n      = sta_->findPathEnds(exception_from,            /* from */\n                           nullptr,               /* thrus */\n                           exception_to,          /* to */\n                           true,                  /* unconstrained */\n                           scenes,                /* scenes */\n                           sta::MinMaxAll::max(), /* min_max */\n                           1,                     /* group_path_count */\n                           1,                     /* endpoint_path_count */\n                           false,                 /* unique_pins */\n                           false,                 /* unique_edges */\n                           -sta::INF,             /* slack_min */\n                           sta::INF,              /* slack_max */\n                           true,                  /* sort_by_slack */\n                           group_names,           /* group_names */\n                           true,                  /* setup */\n                           false,                 /* hold */\n                           false,                 /* recovery */\n                           false,                 /* removal */\n                           true,                  /* clk_gating_setup */\n                           false);                /* clk_gating_hold */\n\n  float setup_delay\n      = setup_ends.empty()\n            ? 0.0\n            : static_cast<float>(setup_ends[0]->dataArrivalTime(sta_));\n\n  // Find worst unconstrained hold path delay/shortest path\n  sta::PathEndSeq hold_ends\n      = sta_->findPathEnds(exception_from,             /* from */\n                           nullptr,               /* thrus */\n                           exception_to,               /* to */\n                           true,                  /* unconstrained */\n                           scenes,                /* scenes */\n                           sta::MinMaxAll::min(), /* min_max */\n                           1,                     /* group_path_count */\n                           1,                     /* endpoint_path_count */\n                           false,                 /* unique_pins */\n                           false,                 /* unique_edges */\n                           -sta::INF,             /* slack_min */\n                           sta::INF,              /* slack_max */\n                           true,                  /* sort_by_slack */\n                           group_names,           /* group_names */\n                           false,                 /* setup */\n                           true,                  /* hold */\n                           false,                 /* recovery */\n                           false,                 /* removal */\n                           false,                 /* clk_gating_setup */\n                           true);                 /* clk_gating_hold */\n\n  float hold_delay\n      = hold_ends.empty()\n            ? 0.0\n            : static_cast<float>(hold_ends[0]->dataArrivalTime(sta_));\n\n  sta::Unit* time_unit = sta_->units()->timeUnit();\n  logger_->info(RAM,\n                44,\n                \"RAM maximum path delay: {} {}\",\n                time_unit->asString(setup_delay),\n                time_unit->scaleAbbrevSuffix());\n  logger_->info(RAM,\n                45,\n                \"RAM minimum path delay: {} {}\",\n                time_unit->asString(hold_delay),\n                time_unit->scaleAbbrevSuffix());\n\n  for (auto* end : setup_ends) {\n    delete end;\n  }\n  for (auto* end : hold_ends) {\n    delete end;\n  }\n  delete exception_from;\n  delete exception_to;\n\n  if (setup_delay <= 0.0F) {\n    return 0.0;\n  }\n  logger_->info(RAM,\n                47,\n                \"Using maximum path delay of {} {} as the clock period for \"\n                \"power estimation\",\n                time_unit->asString(setup_delay),\n                time_unit->scaleAbbrevSuffix());\n  return time_unit->staToUser(setup_delay);\n}

[rovinski]

@tnguy19 this is probably correct - many STA objects have manually controlled lifetimes and need to be deleted.

[tnguy19]

Thanks for pointing this out. I investigated this by doing what Gemini suggested but couldnt get it to work properly.
I tried explicitly deleting the exception objects but it crashed with a segfault in reportTiming(). It looks like OpenSTA frees these objects itself and the program crashed because they were deleted twice

[rovinski]

Can you run with asan and see if it's properly freed? Alternatively use valgrind.

bazelisk build --config=asan :openroad

[tnguy19]

I ran the check on the 8x8 sky130 test with asan and the only leaks that are RAM-related come from ram::RamGen::ramPdngen and ram::ram_pdngen , and nothing from reportTiming() or the exception/path end code is in the report

[tnguy19]

@maliberty All CI tests passed, ready for review

[tnguy19]

@maliberty merge conflict is resolved, ready for review