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

Mapper refactoring #676

Merged
merged 3 commits into from
Apr 14, 2023
Merged

Mapper refactoring #676

merged 3 commits into from
Apr 14, 2023

Conversation

magnatelee
Copy link
Contributor

Per the internal discussion, this PR implements the following changes to the Legate mapper:

  • Libraries can now have only one mapper and should provide their mappers at the registration time.
  • When no mapper is given, the default mapper will be used for the library.
  • Mapper id parameters have been removed from the APIs where they were expected.

This PR builds on #675 and so it currently inherits all of the changes. I will rebase this PR once #675 is merged.

@magnatelee magnatelee added the category:improvement PR introduces an improvement and will be classified as such in release notes label Apr 11, 2023
* Libraries can now have only one mapper that should be provided
  at the registration time
* If no mapper is given, the default mapper will be used for
  the library
* All mapper id parameters are removed from the APIs
Copy link

@jjwilke jjwilke left a comment

Choose a reason for hiding this comment

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

LGTM. A couple minor nits.

TaskTarget task_target(const Task& task, const std::vector<TaskTarget>& options);
std::vector<StoreMapping> store_mappings(const Task& task,
const std::vector<StoreTarget>& options);
virtual Scalar tunable_value(TunableID tunable_id);
Copy link

Choose a reason for hiding this comment

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

Does this need to be marked virtual?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my initial thought is that there could be some mappers that would only have custom tunables (though that'd need the machine query interface to be protected), but now I think that'd rather need a custom mapper. let me remove that.


public:
void set_machine(const MachineQueryInterface* machine);
TaskTarget task_target(const Task& task, const std::vector<TaskTarget>& options);
Copy link

Choose a reason for hiding this comment

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

Should these be marked override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, I forgot to put those. Will add them.

@magnatelee magnatelee merged commit 77e0fb7 into nv-legate:branch-23.05 Apr 14, 2023
@magnatelee magnatelee deleted the mapper-pr branch April 14, 2023 21:00
rapids-bot bot pushed a commit to rapidsai/kvikio that referenced this pull request May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:improvement PR introduces an improvement and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants