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

misc: remove MPIR_Context_id_t #7225

Merged
merged 2 commits into from
Dec 5, 2024
Merged

misc: remove MPIR_Context_id_t #7225

merged 2 commits into from
Dec 5, 2024

Conversation

hzhou
Copy link
Contributor

@hzhou hzhou commented Nov 26, 2024

Pull Request Description

[This is one step toward merging #6189]

Using shorter integer type for context id has little benefit since nearly all expressions will default to int and then the type truncation will be either ignored or cast away, thus serve no additional safety. It only creates noise when we do want to check conversion warnings.

The original purpose (according to comments) is to conserve header space in active messages. We still can do that, just add cast -- better yet, add bound check -- when we fill the am header.

Thus, replace MPIR_Context_id_t with int for simpler code.

[skip warnings]

Author Checklist

  • Provide Description
    Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
  • Commits Follow Good Practice
    Commits are self-contained and do not do two things at once.
    Commit message is of the form: module: short description
    Commit message explains what's in the commit.
  • Passes All Tests
    Whitespace checker. Warnings test. Additional tests via comments.
  • Contribution Agreement
    For non-Argonne authors, check contribution agreement.
    If necessary, request an explicit comment from your companies PR approval manager.

Using shorter integer type for context id has little benefit since
nearly all expressions will default to int and then the type truncation
will be either ignored or cast away, thus serve no additional safety. It
only creates noise when we do want to check conversion warnings.

The original purpose (according to comments) is to conserve header space
in active messages. We still can do that, just add cast -- better yet,
add bound check -- when we fill the am header.

Thus, replace MPIR_Context_id_t with int for simpler code.
@hzhou
Copy link
Contributor Author

hzhou commented Nov 26, 2024

test:mpich/ch3/most
test:mpich/ch4/most

@hzhou hzhou requested a review from raffenet November 26, 2024 21:02
Optimize struct MPIDI_Message_match_parts to be 64-bit according to the
design goal noted in the comment.
@hzhou
Copy link
Contributor Author

hzhou commented Dec 4, 2024

test:mpich/ch3/most

Copy link
Contributor

@raffenet raffenet left a comment

Choose a reason for hiding this comment

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

LGTM

@hzhou hzhou merged commit 8c5b354 into pmodels:main Dec 5, 2024
6 checks passed
@hzhou hzhou deleted the 2411_context_id_t branch December 5, 2024 18:31
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.

2 participants