drt: PA minor refactoring #5744

bnmfw · 2024-09-13T19:47:30Z

[Context] This used to be a huge, overwhelming PR, Its contents were splits in PRs #5760 #5770 #5778 #5783 and #5788. As those were merged this PR was updated to not included their changes ('drt: master merge' commits). What remains can be reviewed as a single PR.

This PR contains minor refactoring changes do the PA module

Straight Forward Refactoring Decisions:

some variables still needed snake_case.
some spaces were added
some comments were changed
Doxygen was added to some functions missing it
genAPEnclosedBoundary was moved to be closer to genAPCentered and genAPOnTrack which are very similar functions
in mergePinShapes and auxilary variable tech was created to replace getDesign()->getTech() which was called some times

Other Refactoring Decisions:

Some minor parts of code were changed for a less verbose equivalent, here are they:

Initialize a vector with set size:

std::vector<gtl::polygon_90_set_data<frCoord>> pin_shapes;
...
pin_shapes.clear();
pin_shapes.resize(getDesign()->getTech()->getLayers().size());

changed to

std::vector<gtl::polygon_90_set_data<frCoord>> pin_shapes(num_layers);

Using find directly in if statement:

auto it = coords.find(coord);
if (it == coords.end()) {

changed to

if (coords.find(coord) == coords.end()) {

Cutting unnecessary if statement

if (valid_via_defs.empty()) {
  ap->setAccess(dir, false);
} else {
  ap->setAccess(dir, true);
}

changed to

ap->setAccess(dir, !valid_via_defs.empty());

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-13T20:01:14Z

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

src/drt/src/pa/FlexPA_prep.cpp

QuantamHD · 2024-09-14T16:13:11Z

src/drt/src/pa/FlexPA_prep.cpp

+  bool use_center_line = false;
+  if (is_macro_cell_pin) {
+    auto rect_dir = gtl::guess_orientation(rect);
+    if ((rect_dir == gtl::HORIZONTAL && is_layer1_horz)


I feel like this would benefit from a helper that said LayerMatchesRectOrentation(layer, rect)

I'm noting this for the subsequent PR. For this one as it became pretty big I do not intend to create new functions

src/drt/src/pa/FlexPA.cpp

src/drt/src/pa/FlexPA.h

rovinski · 2024-09-16T04:54:30Z

src/drt/src/pa/FlexPA.h

+   *
+   * @param pin pin object which will have figures merged by layer
+   * @param inst_term instance terminal from which to get xfrom
+   * @param is_shrink if polygons will be shrinked


Suggested change

* @param is_shrink if polygons will be shrinked

* @param is_shrink if polygons will be shrunk

Could you also describe further how it will be shrunk? Something like "if True, pin's polygons will be shrunk by ...."

I'm not completely sure why there is shrinking. Is is_shrink == True then the input rectangle will have the side in the non preferred direction of the layer reduced by half the layer width. I'm not sure why this is the case

src/drt/src/pa/FlexPA_unique.h

osamahammad21

I have only looked at FlexPA.h and FlexPA.cpp

src/drt/src/pa/FlexPA.h

osamahammad21 · 2024-09-16T12:28:48Z

src/drt/src/pa/FlexPA.h

+   *
+   * @param pin pin object which will have figures merged by layer
+   * @param inst_term instance terminal from which to get xfrom
+   * @param is_shrink if polygons will be shrinked


Could you also describe further how it will be shrunk? Something like "if True, pin's polygons will be shrunk by ...."

src/drt/src/pa/FlexPA.h

Signed-off-by: bernardo <[email protected]>

…ROAD into drt_minor_refactoring

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-16T19:07:17Z

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

github-actions · 2024-09-16T19:09:04Z

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

github-actions · 2024-09-16T19:11:54Z

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

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-20T16:36:22Z

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

github-actions · 2024-09-20T16:47:50Z

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

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-22T15:53:23Z

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

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-23T13:32:42Z

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

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-23T17:54:35Z

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

github-actions · 2024-09-25T14:13:15Z

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

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-25T14:21:15Z

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

github-actions · 2024-09-25T14:26:23Z

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

src/drt/src/pa/FlexPA.h

src/drt/src/pa/FlexPA_init.cpp

osamahammad21 · 2024-09-17T13:36:19Z

src/drt/src/pa/FlexPA_prep.cpp

-    layerWidths.resize(getDesign()->getTech()->getLayers().size(), 0);
-    for (int i = 0; i < int(layerWidths.size()); i++) {
-      layerWidths[i] = getDesign()->getTech()->getLayer(i)->getWidth();
+  frTechObject* tech = getDesign()->getTech();


better if we have a getTech function in FlexPA. not needed in this PR. just a note

src/drt/src/pa/FlexPA_prep.cpp

src/drt/src/pa/FlexPA.h

src/drt/src/pa/FlexPA_prep.cpp

osamahammad21 · 2024-09-26T10:18:40Z

src/drt/src/pa/FlexPA_prep.cpp

+void FlexPA::genAPEnclosedBoundary(std::map<frCoord, frAccessPointEnum>& coords,
+                                   const gtl::rectangle_data<frCoord>& rect,
+                                   const frLayerNum layer_num,
+                                   const bool is_curr_layer_horz)


Can you move to the same place so that the changes can be seen? (after gen_initializeAccessPoints)
unless there is a reason for moving it here.

There are no changes in genAPEnclosedBoundary, but it is one of the three functions that generate access points coordinates for a given type (genAPOnTrack, genAPCentered, genAPEnclosedBoundary). I just moved it to be right after the other two instead of being far down.

src/drt/src/pa/FlexPA_prep.cpp

github-actions · 2024-09-26T14:41:03Z

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

Signed-off-by: bernardo <[email protected]>

github-actions · 2024-09-26T20:42:18Z

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

drt: PA minor refactoring and doxygen

ddfaea6

Signed-off-by: bernardo <[email protected]>

bnmfw added documentation Improvements or additions to documentation drt Detailed Routing labels Sep 13, 2024

bnmfw requested a review from osamahammad21 September 13, 2024 19:53

bnmfw mentioned this pull request Sep 13, 2024

drt: minor refactoring #5701

Closed

QuantamHD reviewed Sep 14, 2024

View reviewed changes

rovinski reviewed Sep 16, 2024

View reviewed changes

osamahammad21 requested changes Sep 16, 2024

View reviewed changes

bnmfw added 3 commits September 16, 2024 18:51

drt: PA minor refactoring adressing reviews 1

d44bd05

Signed-off-by: bernardo <[email protected]>

Merge branch 'master' of https://github.com/The-OpenROAD-Project/Open…

3ad291a

…ROAD into drt_minor_refactoring

drt: deleted debug comment

e69adc2

Signed-off-by: bernardo <[email protected]>

bnmfw force-pushed the drt_minor_refactoring branch from 33aa9f4 to e69adc2 Compare September 16, 2024 18:58

bnmfw requested a review from osamahammad21 September 16, 2024 18:59

drt: master merge

6c40d86

Signed-off-by: bernardo <[email protected]>

bnmfw force-pushed the drt_minor_refactoring branch from 203ff42 to 6c40d86 Compare September 20, 2024 16:34

drt: master merge

5008069

Signed-off-by: bernardo <[email protected]>

bnmfw added 2 commits September 23, 2024 13:07

Merge remote-tracking branch 'origin/master' into drt_minor_refactoring

91360d5

drt: master merge

fe4926d

Signed-off-by: bernardo <[email protected]>

bnmfw marked this pull request as draft September 23, 2024 14:28

drt: master merge

882a411

Signed-off-by: bernardo <[email protected]>

bnmfw force-pushed the drt_minor_refactoring branch from 3367a9b to e563552 Compare September 25, 2024 14:12

drt: master merge

ba0b104

Signed-off-by: bernardo <[email protected]>

bnmfw force-pushed the drt_minor_refactoring branch from e563552 to ba0b104 Compare September 25, 2024 14:18

bnmfw changed the title ~~drt: PA minor refactoring and doxygen~~ drt: PA minor refactoring Sep 25, 2024

bnmfw marked this pull request as ready for review September 25, 2024 14:40

osamahammad21 requested changes Sep 26, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into drt_minor_refactoring

625c965

bnmfw requested a review from osamahammad21 September 26, 2024 14:43

bnmfw added 2 commits September 26, 2024 20:32

drt: better detailed documentation and minor adjusments

f252387

Signed-off-by: bernardo <[email protected]>

Merge remote-tracking branch 'origin/master' into drt_minor_refactoring

4f92caf

bnmfw force-pushed the drt_minor_refactoring branch from bc67868 to 4f92caf Compare September 26, 2024 20:34

bnmfw mentioned this pull request Sep 28, 2024

drt: genApEnclosedBoundary code duplication elimination #5822

Merged

osamahammad21 approved these changes Sep 29, 2024

View reviewed changes

osamahammad21 merged commit bfe2c72 into The-OpenROAD-Project:master Sep 29, 2024
10 checks passed

bnmfw deleted the drt_minor_refactoring branch September 29, 2024 15:50

bnmfw mentioned this pull request Dec 4, 2024

DRT PA Refactor #5721

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drt: PA minor refactoring #5744

drt: PA minor refactoring #5744

bnmfw commented Sep 13, 2024 •

edited

Loading

github-actions bot commented Sep 13, 2024

QuantamHD Sep 14, 2024

bnmfw Sep 16, 2024 •

edited

Loading

rovinski Sep 16, 2024

osamahammad21 Sep 16, 2024

bnmfw Sep 16, 2024

osamahammad21 left a comment

osamahammad21 Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 22, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

osamahammad21 Sep 17, 2024

osamahammad21 Sep 26, 2024

bnmfw Sep 26, 2024 •

edited

Loading

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

	* @param is_shrink if polygons will be shrinked
	* @param is_shrink if polygons will be shrunk

drt: PA minor refactoring #5744

drt: PA minor refactoring #5744

Conversation

bnmfw commented Sep 13, 2024 • edited Loading

Straight Forward Refactoring Decisions:

Other Refactoring Decisions:

Initialize a vector with set size:

Using find directly in if statement:

Cutting unnecessary if statement

github-actions bot commented Sep 13, 2024

QuantamHD Sep 14, 2024

Choose a reason for hiding this comment

bnmfw Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

rovinski Sep 16, 2024

Choose a reason for hiding this comment

osamahammad21 Sep 16, 2024

Choose a reason for hiding this comment

bnmfw Sep 16, 2024

Choose a reason for hiding this comment

osamahammad21 left a comment

Choose a reason for hiding this comment

osamahammad21 Sep 16, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 22, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

osamahammad21 Sep 17, 2024

Choose a reason for hiding this comment

osamahammad21 Sep 26, 2024

Choose a reason for hiding this comment

bnmfw Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

bnmfw commented Sep 13, 2024 •

edited

Loading

bnmfw Sep 16, 2024 •

edited

Loading

bnmfw Sep 26, 2024 •

edited

Loading