Skip to content

Conversation

@openroad-ci
Copy link
Collaborator

This PR adds a new command to MPL: set_macro_halo, which sets a halo for a specified macro. Currently, this halo works only for placement, the blockages generated later still use the values in MACRO_PLACE_HALO (or MACRO_BLOCKAGE_HALO).

Blockage generation will be updated in an upcoming PR.

Copy link
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 a new command, set_macro_halo, which allows setting specific halos for individual macros. The implementation correctly integrates this feature into the macro placement flow, with logic to use these specific halos or fall back to global defaults. The changes are well-structured, including updates to the TCL interface, core placement logic, and GUI visualization. My review identified a couple of minor issues: the documentation for the new command in README.md is incorrect due to a copy-paste error, and there's a small redundancy in the new TCL command's implementation. Addressing these will improve the clarity and correctness of the new feature.

Comment on lines 130 to 140
set_macro_guidance_region
-macro_name macro_name
-region region
```

#### Options

| Switch Name | Description |
| ----- | ----- |
| `-macro_name` | The name of a macro of the design. |
| `-region` | The lower left corner and upper right corner {lx ly ux uy} of the region in microns. |
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The documentation for the new set_macro_halo command appears to be a copy-paste from set_macro_guidance_region and is incorrect. The command name, arguments, and descriptions should be updated to reflect the new functionality.

Specifically:

  • The command in the example should be set_macro_halo.
  • The argument should be -halo instead of -region.
  • The description for -halo should state that it's a list of two values: {width height} in microns.
Suggested change
set_macro_guidance_region
-macro_name macro_name
-region region
```
#### Options
| Switch Name | Description |
| ----- | ----- |
| `-macro_name` | The name of a macro of the design. |
| `-region` | The lower left corner and upper right corner {lx ly ux uy} of the region in microns. |
set_macro_halo
-macro_name macro_name
-halo {width height}

Options

Switch Name Description
-macro_name The name of a macro of the design.
-halo The horizontal and vertical halo {width height} around the macro in microns.

Comment on lines +312 to +313
set width $width
set height $height
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The lassign command on the previous line already assigns the values from the halo list to the width and height variables. These set commands are redundant and can be removed.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions


void setMacroPlacementFile(const std::string& file_name);
void addGuidanceRegion(odb::dbInst* macro, odb::Rect region);
void addMacroHalo(odb::dbInst* macro, std::pair<int, int> halo);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::pair" is directly included [misc-include-cleaner]

src/mpl/include/mpl/rtl_mp.h:8:

- #include <vector>
+ #include <utility>
+ #include <vector>

void setHaloHeight(int halo_height);
void setGuidanceRegions(
const std::map<odb::dbInst*, odb::Rect>& guidance_regions);
void setMacroHalos(const std::map<odb::dbInst*, std::pair<int, int>>& halos);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::pair" is directly included [misc-include-cleaner]

src/mpl/src/hier_rtlmp.h:9:

- #include <vector>
+ #include <utility>
+ #include <vector>

guidance_regions_[macro] = region;
}

void MacroPlacer::addMacroHalo(odb::dbInst* macro, std::pair<int, int> halo)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::pair" is directly included [misc-include-cleaner]

src/mpl/src/rtl_mp.cpp:7:

- #include <vector>
+ #include <utility>
+ #include <vector>

Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

odb::dbDatabase* db_ = nullptr;

std::map<odb::dbInst*, odb::Rect> guidance_regions_;
std::map<odb::dbInst*, std::pair<int, int>> macro_halos_;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::pair" is directly included [misc-include-cleaner]

src/mpl/include/mpl/rtl_mp.h:8:

- #include <vector>
+ #include <utility>
+ #include <vector>

Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
@joaomai joaomai requested a review from AcKoucher January 22, 2026 20:03
@github-actions
Copy link
Contributor

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

Copy link
Contributor

@AcKoucher AcKoucher left a comment

Choose a reason for hiding this comment

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

Please, address the code assist comment in the .mpl file.
Also, take a look at why the tests with fixed macros are failing (I suspect it has something to do with the new method for creating HardMacros).

Comment on lines 224 to 230
void MacroPlacer::addMacroHalo(odb::dbInst* macro,
int halo_width,
int halo_height)
{
hier_rtlmp_->addMacroHalo(macro, halo_width, halo_height);
}

Copy link
Contributor

@AcKoucher AcKoucher Jan 23, 2026

Choose a reason for hiding this comment

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

I think the name setMacroHalo makes more sense, as a macro won't have more than one Halo.

}

void
add_macro_halo(odb::dbInst* macro,
Copy link
Contributor

@AcKoucher AcKoucher Jan 23, 2026

Choose a reason for hiding this comment

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

set_macro_halo


std::map<std::string, odb::Rect> fences_; // macro_name, fence
std::map<odb::dbInst*, odb::Rect> guides_; // Macro -> Guidance Region
std::map<odb::dbInst*, HardMacro::Halo>
Copy link
Contributor

Choose a reason for hiding this comment

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

macro_to_halo_ and drop the comment.


void setMacroPlacementFile(const std::string& file_name);
void addGuidanceRegion(odb::dbInst* macro, odb::Rect region);
void addMacroHalo(odb::dbInst* macro, int halo_width, int halo_height);
Copy link
Contributor

@AcKoucher AcKoucher Jan 23, 2026

Choose a reason for hiding this comment

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

setMacroHalo

const int width = master->getWidth() + (2 * tree_->halo_width);
const int height = master->getHeight() + (2 * tree_->halo_height);
macro_with_halo_area += (width * static_cast<int64_t>(height));
macro_with_halo_area += tree_->maps.inst_to_hard[unfixed_macro]->getArea();
Copy link
Contributor

Choose a reason for hiding this comment

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

Use std::map::at as the map is not being modified.

clustering_engine_ = std::make_unique<ClusteringEngine>(
block_, network_, logger_, tritonpart_, graphics_.get());

createHardMacros(block_->getTopModule());
Copy link
Contributor

Choose a reason for hiding this comment

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

I liked that you separated this in one function :-)

I've been trying to make the ClusteringEngine the object that populates/changes the tree_.
That said, I think this method should belong to the ClusteringEngine and not to HierRTLMP. (Ideally it should belong to a placement engine, but that is for one day in the future when the stars align).

Comment on lines +457 to +473
odb::Rect halo_bbox(macro.getX(),
macro.getY(),
macro.getX() + macro.getWidth(),
macro.getY() + macro.getHeight());
odb::Rect macro_bbox(macro.getRealX(),
macro.getRealY(),
macro.getRealX() + width,
macro.getRealY() + height);

painter.drawRect(bbox);
painter.drawString(bbox.xCenter(),
bbox.yCenter(),
halo_bbox.moveDelta(outline_.xMin(), outline_.yMin());
macro_bbox.moveDelta(outline_.xMin(), outline_.yMin());

painter.setBrush(gui::Painter::kDarkRed);
painter.drawRect(halo_bbox);

painter.setBrush(gui::Painter::kRed);
painter.drawRect(macro_bbox);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice!

@AcKoucher
Copy link
Contributor

The new code from #9362 will require changes in this PR.

Signed-off-by: João Mai <jmai@precisioninno.com>
@joaomai joaomai requested a review from AcKoucher January 28, 2026 21:45
@github-actions
Copy link
Contributor

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

Copy link
Contributor

@AcKoucher AcKoucher left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +2496 to +2497
auto macro_insts
= block_->getInsts() | std::ranges::views::filter(&odb::dbInst::isBlock);
Copy link
Contributor

Choose a reason for hiding this comment

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

cool

@joaomai joaomai requested a review from maliberty January 29, 2026 14:45
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.

3 participants