drt: genPatterns_commit refactoring #5872

bnmfw · 2024-10-04T00:36:10Z

Some refactoring on genPatterns_commit is being done to improve readability and reduce code.

Access Patterns Graph Backwards Path Function and Readability

The first part of genPatterns_commit is a loop that traces a path through access points of an instance that represent a chosen access pattern.

Right now the loop is a little over complicated for what it does, so I refactored it a bit to make more obvious.

Added the variables source_node_idx and drain_node_idx to make the start and end points of the path search more clear, instead decreasing pin_cnt.

This was transformed in a function called extractAccessPatternFromNodes as it has a pretty distinct role and function.

Used Pattern Guard

The later part of the had an if statement to check is the generated pattern was unused, if true it did more checks, later it essentially had an else with return false. This as transformed into a guard.

No further changes on this function will be done in this PR as the git diff shows everything past the guard as changed due to the indentation level change. Further changes will be at a separate PR.

github-actions · 2024-10-04T00:41:10Z

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

github-actions · 2024-10-04T15:47:03Z

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

github-actions

clang-tidy made some suggestions

src/drt/src/pa/FlexPA.h

github-actions

clang-tidy made some suggestions

src/drt/src/pa/FlexPA.h

github-actions · 2024-10-08T12:02:35Z

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

github-actions · 2024-10-16T20:50:57Z

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

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

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

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

github-actions · 2024-10-18T17:44:58Z

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

rovinski · 2024-10-23T04:31:43Z

src/drt/src/pa/FlexPA_prep.cpp

+          leftAP = ap;
+          leftPt = tmpPt.x();
+        }
+        if (tmpPt.x() > rightPt) {


Should this be else if?

nope. Consider the first iteration. we need to set both; leftPt and rightPt, to tmpPt

rovinski · 2024-10-23T04:32:05Z

src/drt/src/pa/FlexPA_prep.cpp

      }
    }
+    if (n_no_ap_pins == inst_term->getTerm()->getPins().size()) {
+      logger_->error(DRT, 91, "Pin does not have valid ap.");


Capitalize AP. Also, can you add the term name to the print?

rovinski · 2024-10-23T04:34:43Z

src/drt/src/pa/FlexPA_prep.cpp

+  const int drain_node_idx = getFlatIdx(pins.size(), 0, max_access_point_size);
+  auto drain_node = &(nodes[drain_node_idx]);
+  int curr_node_idx = drain_node->getPrevNodeIdx();


Does "drain" have a special meeting here? I the opposite of source is usually "sink" in these contexts

I guess sink is the correct word in this context.

rovinski · 2024-10-23T04:34:58Z

src/drt/src/pa/FlexPA.h

@@ -568,6 +568,25 @@ class FlexPA
                  int curr_unique_inst_idx,
                  int max_access_point_size);

+  /**
+   * @brief Extracts the access patterns given the graph nodes composing the
+   * access points relatioship


Suggested change

* access points relatioship

* access points relationship

rovinski · 2024-10-23T04:35:51Z

src/drt/src/pa/FlexPA.h

+   * access points relatioship
+   *
+   * @param nodes {pin,access_point} nodes of the access pattern graph
+   * @param pins vector os pins of the unique instance


vector of pins?

rovinski · 2024-10-23T04:37:39Z

src/drt/src/pa/FlexPA.h

+   * @param max_access_point_size number of acc points the instance with most
+   * acc points has


Suggested change

* @param max_access_point_size number of acc points the instance with most

* acc points has

* @param max_access_point_size the maximum number of access points across

* all instances

rovinski · 2024-10-23T04:42:24Z

src/drt/src/pa/FlexPA_prep.cpp

+        if (owners.find(inst_term->getNet()) != owners.end()) {
+          viol_access_points.insert(std::make_pair(idx_1, idx_2));  // idx ;
        }
-      }
-      if (n_no_ap_pins == inst_term->getTerm()->getPins().size()) {
-        logger_->error(DRT, 91, "Pin does not have valid ap.");
-      }
-    }
-    pin_access_pattern->setBoundaryAP(true, left_access_point);
-    pin_access_pattern->setBoundaryAP(false, right_access_point);
-
-    std::set<frBlockObject*> owners;
-    if (target_obj != nullptr
-        && genPatterns_gc({target_obj}, objs, Commit, &owners)) {
-      pin_access_pattern->updateCost();
-      unique_inst_patterns_[curr_unique_inst_idx].push_back(
-          std::move(pin_access_pattern));
-      // genPatterns_print(nodes, pins, max_access_point_size);
-      is_valid = true;
-    } else {
-      for (int idx_1 = 0; idx_1 < (int) pins.size(); idx_1++) {
-        auto idx_2 = access_pattern[idx_1];
-        auto& [pin, inst_term] = pins[idx_1];
-        if (inst_term->hasNet()) {
-          if (owners.find(inst_term->getNet()) != owners.end()) {
-            viol_access_points.insert(std::make_pair(idx_1, idx_2));  // idx ;
-          }
-        } else {
-          if (owners.find(inst_term) != owners.end()) {
-            viol_access_points.insert(std::make_pair(idx_1, idx_2));  // idx ;
-          }
+      } else {
+        if (owners.find(inst_term) != owners.end()) {


I wonder if there's a more efficient way to do this because this will traverse owners twice. It might not matter if the else is rarely exercised or if owners is small.

osamahammad21 · 2024-10-27T15:32:18Z

src/drt/src/pa/FlexPA_prep.cpp

+
+  const int source_node_idx = getFlatIdx(-1, 0, max_access_point_size);
+  const int drain_node_idx = getFlatIdx(pins.size(), 0, max_access_point_size);
+  auto drain_node = &(nodes[drain_node_idx]);


Suggested change

auto drain_node = &(nodes[drain_node_idx]);

const auto& drain_node = nodes[drain_node_idx];

use const reference to the object instead of pointer.

osamahammad21 · 2024-10-27T15:33:46Z

src/drt/src/pa/FlexPA_prep.cpp

+  const int drain_node_idx = getFlatIdx(pins.size(), 0, max_access_point_size);
+  auto drain_node = &(nodes[drain_node_idx]);
+  int curr_node_idx = drain_node->getPrevNodeIdx();


I guess sink is the correct word in this context.

osamahammad21 · 2024-10-27T15:34:23Z

src/drt/src/pa/FlexPA_prep.cpp

+      logger_->error(DRT, 90, "Valid access pattern not found.");
+    }
+
+    auto curr_node = &(nodes[curr_node_idx]);


osamahammad21 · 2024-10-27T15:48:43Z

src/drt/src/pa/FlexPA_prep.cpp

+  frAccessPoint* leftAP = nullptr;
+  frAccessPoint* rightAP = nullptr;
+  frCoord leftPt = std::numeric_limits<frCoord>::max();
+  frCoord rightPt = std::numeric_limits<frCoord>::min();


why the renaming? this is not the followed style.

osamahammad21 · 2024-10-27T15:51:18Z

src/drt/src/pa/FlexPA_prep.cpp

+          leftAP = ap;
+          leftPt = tmpPt.x();
+        }
+        if (tmpPt.x() > rightPt) {


nope. Consider the first iteration. we need to set both; leftPt and rightPt, to tmpPt

bnmfw force-pushed the drt_genPatterns_commit_refactoring branch from 2ebcfdb to 91a9156 Compare October 4, 2024 15:41

github-actions bot reviewed Oct 4, 2024

View reviewed changes

src/drt/src/pa/FlexPA.h Outdated Show resolved Hide resolved

bnmfw force-pushed the drt_genPatterns_commit_refactoring branch from b050b9b to 63dc4f8 Compare October 8, 2024 11:56

github-actions bot reviewed Oct 8, 2024

View reviewed changes

src/drt/src/pa/FlexPA.h Show resolved Hide resolved

bnmfw mentioned this pull request Oct 8, 2024

drt: Better Dynamic Programming (DP) Nodes and Elimination of getNestedIdx #5901

Draft

bnmfw force-pushed the drt_genPatterns_commit_refactoring branch from 60ed9ab to cc3f2a2 Compare October 16, 2024 20:46

bnmfw added 4 commits October 18, 2024 17:37

drt: backwards pattern propagation readability

fdc7306

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

drt: used access pattern guard

4cc7bcb

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

drt: extraction of access patterns from nodes separated in its own fu…

4f92b89

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

clang-tidy

f7b2789

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

bnmfw force-pushed the drt_genPatterns_commit_refactoring branch from cc3f2a2 to f7b2789 Compare October 18, 2024 17:40

bnmfw marked this pull request as ready for review October 18, 2024 17:41

bnmfw requested a review from osamahammad21 October 18, 2024 17:42

rovinski reviewed Oct 23, 2024

View reviewed changes

osamahammad21 requested changes Oct 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drt: genPatterns_commit refactoring #5872

drt: genPatterns_commit refactoring #5872

bnmfw commented Oct 4, 2024 •

edited

Loading

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot left a comment

github-actions bot left a comment

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 16, 2024

github-actions bot commented Oct 18, 2024

rovinski Oct 23, 2024

osamahammad21 Oct 27, 2024

rovinski Oct 23, 2024

rovinski Oct 23, 2024

osamahammad21 Oct 27, 2024

rovinski Oct 23, 2024

rovinski Oct 23, 2024

rovinski Oct 23, 2024

rovinski Oct 23, 2024

osamahammad21 Oct 27, 2024

osamahammad21 Oct 27, 2024

osamahammad21 Oct 27, 2024

osamahammad21 Oct 27, 2024

osamahammad21 Oct 27, 2024

		* @param max_access_point_size number of acc points the instance with most
		* acc points has

	auto drain_node = &(nodes[drain_node_idx]);
	const auto& drain_node = nodes[drain_node_idx];

drt: genPatterns_commit refactoring #5872

Are you sure you want to change the base?

drt: genPatterns_commit refactoring #5872

Conversation

bnmfw commented Oct 4, 2024 • edited Loading

Access Patterns Graph Backwards Path Function and Readability

Used Pattern Guard

github-actions bot commented Oct 4, 2024

github-actions bot commented Oct 4, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 16, 2024

github-actions bot commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnmfw commented Oct 4, 2024 •

edited

Loading