Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drt: curr_idx name changes #5862

Conversation

bnmfw
Copy link
Contributor

@bnmfw bnmfw commented Oct 2, 2024

This PR used to change how getNestedIdx inputs and outputs data, but since the function will be eliminated further down the line I reverted it. Now this PR only contains variable changes.

Also changes instances of
curr_idx_1, curr_idx_2 to either
curr_pin_idx, curr_acc_point_idx or
curr_inst_idx, curr_acc_pattern_idx
depending on the appropriate use.

Similar changes were applied to variables with the same format that have prev or prev_prev instead of curr.

@bnmfw bnmfw added the drt Detailed Routing label Oct 2, 2024
Copy link
Contributor

github-actions bot commented Oct 2, 2024

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

Comment on lines 2857 to 2859
const int idx_1 = flat_idx / idx_2_dim - 1;
const int idx_2 = flat_idx % idx_2_dim;
return {idx_1, idx_2};
Copy link
Member

Choose a reason for hiding this comment

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

Any idea where there is a '-1' here? It seems non-idiomatic.

Copy link
Contributor Author

@bnmfw bnmfw Oct 2, 2024

Choose a reason for hiding this comment

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

Yes. This function is the reciprocal to getFlatIdx:

int FlexPA::getFlatIdx(const int idx_1, const int idx_2, const int idx_2_dim)
{
  return ((idx_1 + 1) * idx_2_dim + idx_2);
}

The -1 inverts the +1 applied to idx_1. The +1, in turn, is there because the minimal value for idx_1 is -1, not 0. So the returned value will be always positive.

-1 is used as an argument for this function at genPatterns_reset, genPatternsInitand genInstRowPatternInit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually don't like how the code keeps flattening and nesting these indexes, it happens a lot and some times one right after the other. If I find a way to eliminate it and keep a single representation for these indexes I will implement it.

@bnmfw bnmfw marked this pull request as draft October 2, 2024 23:14
@bnmfw
Copy link
Contributor Author

bnmfw commented Oct 2, 2024

Just understood what curr_idx_1 and curr_idx_2 are and they are terrible names so I'm changing them and including the change in this PR

Signed-off-by: bernardo <[email protected]>
@bnmfw bnmfw marked this pull request as ready for review October 2, 2024 23:57
@bnmfw bnmfw changed the title drt: getNestedIdx explicit output drt: getNestedIdx explicit output and curr_idx name changes Oct 2, 2024
Copy link
Contributor

github-actions bot commented Oct 2, 2024

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

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

github-actions bot commented Oct 4, 2024

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

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

github-actions bot commented Oct 4, 2024

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

Copy link
Member

@osamahammad21 osamahammad21 left a comment

Choose a reason for hiding this comment

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

I am running an ISPD CI and a secure CI. merging when they pass.

for (int curr_idx_1 = 0; curr_idx_1 <= (int) insts.size(); curr_idx_1++) {
for (int curr_idx_2 = 0; curr_idx_2 < ACCESS_PATTERN_END_ITERATION_NUM;
curr_idx_2++) {
for (int curr_pin_idx = 0; curr_pin_idx <= (int) insts.size();
Copy link
Member

Choose a reason for hiding this comment

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

why not curr _inst_idx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oversight. I actually have already caught that on a further PR I'm working on, but did not think to fix it here. Nice catch. I'm fixing this an re requesting review.

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

github-actions bot commented Oct 8, 2024

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

@bnmfw
Copy link
Contributor Author

bnmfw commented Oct 10, 2024

I am running an ISPD CI and a secure CI. merging when they pass.

Did the CIs pass?

@bnmfw bnmfw changed the title drt: getNestedIdx explicit output and curr_idx name changes drt: Curr_idx name changes Oct 16, 2024
Copy link
Contributor

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

@bnmfw bnmfw changed the title drt: Curr_idx name changes drt: curr_idx name changes Oct 17, 2024
@osamahammad21 osamahammad21 merged commit c749600 into The-OpenROAD-Project:master Oct 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
drt Detailed Routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants