Skip to content

Add team features to native-multi-image.F90 app #269

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.

Sign up for GitHub

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

Jump to bottom
Open
ktras wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add team features to native-multi-image.F90 app #269

ktras wants to merge 9 commits into from
+61 −1

Conversation

@ktras
Copy link
Collaborator

@ktras ktras commented Nov 11, 2025 •
edited
Loading

This PR will not pass CI (with the current macros being set for flang) until PR #165573 is merged into the llvm-project repo.

@ktras ktras requested a review from bonachea November 11, 2025 22:58
@ktras
Copy link
Collaborator Author

ktras commented Nov 11, 2025

@bonachea I have not used macros for each individual team feature. In my initial draft of adding macros to my changes to this program, I did use macros for each individual team feature. However, particularly because of team_number, the use of macros around each different team number was a lot. I didn't prefer that style, so I ended up with the current strategy. If you feel strongly about the additional macros for each separate team feature, I can update the PR to add them back.

bonachea
bonachea requested changes Nov 12, 2025
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. The overall design looks good, I added a few requests for improvements based on a quick skim

@bonachea
Copy link
Member

bonachea commented Nov 19, 2025

Rebasing to pick-up CI changes

@bonachea bonachea force-pushed the update-multi-image-app branch from 7f5f0d2 to b9a42b3 Compare November 19, 2025 03:29
bonachea
bonachea requested changes Nov 19, 2025
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

Recording one new observation

@bonachea bonachea force-pushed the update-multi-image-app branch from b9a42b3 to 3c38a05 Compare December 2, 2025 00:41
@bonachea
Copy link
Member

bonachea commented Dec 2, 2025

Rebasing to pick-up CI changes

@bonachea bonachea force-pushed the update-multi-image-app branch from 71e87f4 to 01b358b Compare December 8, 2025 22:01
@ktras ktras force-pushed the update-multi-image-app branch from aa765aa to a51917b Compare December 9, 2025 22:17
@ktras ktras requested a review from bonachea December 9, 2025 22:18
bonachea
bonachea requested changes Dec 10, 2025
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

A few other minor nitpicks, otherwise LGTM!

ktras and others added 2 commits December 10, 2025 09:41
@ktras ktras requested a review from bonachea December 10, 2025 18:25
bonachea
bonachea approved these changes Dec 10, 2025
View reviewed changes
Copy link
Member

@bonachea bonachea left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bonachea bonachea bonachea approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ktras @bonachea