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

fixed llvmir usage in create_module method #7024

Merged
merged 4 commits into from
Jun 24, 2021

Conversation

rawwar
Copy link
Contributor

@rawwar rawwar commented May 13, 2021

fixes #7022

using llvmir instead of ir in create_module method of BaseContext

@sklam
Copy link
Member

sklam commented May 13, 2021

Makes me wonder if that method is actually dead code. Why is it never exercised?

@rawwar
Copy link
Contributor Author

rawwar commented May 14, 2021

I see this being used in -

wrapper_module = self.create_module("hsa.kernel.wrapper")

I tried searching for any test cases that make use of this and couldn't so far. @gmarkall can you please help me out?

@gmarkall
Copy link
Member

Makes me wonder if that method is actually dead code. Why is it never exercised?

I believe it's dead, and all target contexts override it.

I also came across this when I turned on flake8 for base.py (in PR #7001).

I see this being used in - ...

This may have bitrotted as the ROCm target is unmaintained - see #6991

@rawwar
Copy link
Contributor Author

rawwar commented May 21, 2021

Shall I remove this method from BaseContext?

@gmarkall
Copy link
Member

I'd be inclined to make it raise NotImplementedError instead, because subclasses of BaseContext should implement it. (Perhaps BaseContext should be an ABC, but I don't think it's worth going to the trouble of making it one for this PR).

@rawwar
Copy link
Contributor Author

rawwar commented May 21, 2021

Sure, i'll make it raise NotImplementedError

numba/core/base.py Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review Effort - short Short size effort needed and removed 3 - Ready for Review labels Jun 21, 2021
Copy link
Member

@esc esc 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 the patch @stuartarchibald @rawwar !

@esc esc added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Jun 24, 2021
@sklam sklam merged commit 1054e88 into numba:master Jun 24, 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 Effort - short Short size effort needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ir not imported
5 participants