Skip to content

UPF: add upf_version#9743

Draft
DivyanshuX9 wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
DivyanshuX9:Upf_5617_issue_resolve_by_Divyanshu
Draft

UPF: add upf_version#9743
DivyanshuX9 wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
DivyanshuX9:Upf_5617_issue_resolve_by_Divyanshu

Conversation

@DivyanshuX9
Copy link
Copy Markdown

@DivyanshuX9 DivyanshuX9 commented Mar 12, 2026

Title: UPF: add upf_version command support

This PR resolves part of Issue #5617 by implementing the upf_version command

Changes made:

  • Added upf_version API in upf.h
  • Implemented command handler in upf.cpp
  • Added SWIG interface binding in upf.i
  • Added Tcl command parsing in upf.tcl

Behavior:

  • Parses upf_version <version>
  • Stores version in dbBlock using setUPFVersion()
  • Emits warning if version string is missing
  • Does not affect existing flow

Implementation strategy:
This follows the incremental approach discussed in Issue #5617:

  • Implement one command at a time
  • Ensure build stability
  • Avoid documentation updates until multiple commands are finalized
  • Keep PR small and reviewable

This PR corresponds to Phase 1:
Basic UPF command support

Next planned commands:

  • create_supply_port
  • create_supply_net
  • connect_supply_net

No documentation files were modified in this PR as per incremental workflow.

Related issue:
#5617

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the upf_version command. The implementation is straightforward, but there's an issue with error handling. Currently, if an invalid version string is provided, the Tcl command succeeds while only printing a warning. My feedback focuses on propagating the error status from the C++ layer to the Tcl script to ensure the command fails correctly, which is more consistent with typical command-line tool behavior.

Comment on lines +223 to +226
void upf_version_cmd(const char* version) {
odb::dbDatabase* db = getOpenRoad()->getDb();
upf::upf_version(getOpenRoad()->getLogger(), db->getChip()->getBlock(), version);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The upf_version_cmd function should propagate the success/failure status from upf::upf_version. This allows the Tcl script to handle errors properly. Please change the return type to bool and return the result of the call.

  bool upf_version_cmd(const char* version) {
    odb::dbDatabase* db = getOpenRoad()->getDb();
    return upf::upf_version(getOpenRoad()->getLogger(), db->getChip()->getBlock(), version);
  }

sta::check_argc_eq1 "upf_version" $args

set version [lindex $args 0]
upf::upf_version_cmd $version
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The Tcl command should check the return value of upf::upf_version_cmd and report an error if it fails. This ensures that invalid calls (e.g., with an empty version string) cause the Tcl command to fail, which is standard practice. This change depends on the suggested change in src/upf/src/upf.i.

  if {![upf::upf_version_cmd $version]} {
    sta::error "Failed to set UPF version."
  }

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Bro I Updated the implementation to propagate the return status
from C++ to Tcl as per your suggestion
Changed upf_version_cmd to return bool, Updated Tcl to check return value, Fixed function signature in header
Also fixed my commit with Signed-off-by for DCO.

Please review again.

@DivyanshuX9 DivyanshuX9 marked this pull request as ready for review March 12, 2026 18:36
@DivyanshuX9 DivyanshuX9 marked this pull request as draft March 12, 2026 18:36
@DivyanshuX9 DivyanshuX9 force-pushed the Upf_5617_issue_resolve_by_Divyanshu branch 3 times, most recently from 3b647ad to 8eab67f Compare March 14, 2026 10:51
Signed-off-by: Divyanshu_Sharma <divyanshu88999@gmail.com>
@DivyanshuX9 DivyanshuX9 force-pushed the Upf_5617_issue_resolve_by_Divyanshu branch from 8eab67f to 3aac0e7 Compare March 14, 2026 11:25
@github-actions
Copy link
Copy Markdown
Contributor

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

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.

1 participant