Skip to content

Refactor hardware extension API to refer to "target" instead. - #6990

Merged
sklam merged 3 commits into
numba:masterfrom
stuartarchibald:wip/refactor_target_extension_api
May 11, 2021
Merged

sklam merged 3 commits into
numba:masterfrom
stuartarchibald:wip/refactor_target_extension_api

Conversation

@stuartarchibald

Copy link
Copy Markdown
Contributor

As title. This is a largely mechanical refactor. It changes the
new hardware extension API to refer to "target" instead of
"hardware". This is because the "hardware" is often actually
synthetic and "target" better describes this.

As title. This is a largely mechanical refactor. It changes the
new hardware extension API to refer to "target" instead of
"hardware". This is because the "hardware" is often actually
synthetic and "target" better describes this.
@stuartarchibald

Copy link
Copy Markdown
Contributor Author

Notes:

  1. This will need a CUDA smoke test because the CUDA target makes use of this API for @overload support.
  2. This PR probably blocks any further refactoring in the @overload area as it renames most of the API in use (it could still be done, the merge conflicts might not be easy to deal with though).

As title. This keeps all the registration mechanics in the same
place.

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good and tests OK with the CUDA target for me locally on Linux.

I've looked through the CUDA changes and they look fine - I've also read through the rest of the PR and it seems OK to the extent that I could understand the changes.

I've made one small suggestion on the diff, but it's not a deal breaker for me. :-)

Comment thread numba/cuda/initialize.py Outdated

cuda_hw = hardware_registry["cuda"]
decorators.jit_registry[cuda_hw] = cuda_jit_device
cuda_hw = target_registry["cuda"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This probably makes more sense as cuda_target now than cuda_hw.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, agree, patched in 2c85862

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@sklam sklam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

While not introduced by this PR, numba/tests/test_target_extension.py can use some refactoring in a future PR. There are code duplicated and unused code in there.

@sklam sklam added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm and removed 3 - Ready for Review labels May 11, 2021
@sklam

sklam commented May 11, 2021

Copy link
Copy Markdown
Member

BFID numba_smoketest_cuda_yaml_57

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels May 11, 2021
@sklam
sklam merged commit 172151d into numba:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants