NVIDIA / numba-cuda

BSD 2-Clause "Simplified" License
24 stars 7 forks source link

let `opt` respect `config.OPT`, also change its default value to `None` #8

Closed dlee992 closed 1 month ago

dlee992 commented 2 months ago

fixes https://github.com/numba/numba/issues/9469

Design: Based on Graham's comment in the original issue, I choose to set opt default value to None, then give users a chance to change opt option for a single jitted func. If users don't specify it, then opt is decided by NUMBA_OPT. The rule is:

  1. if NUMBA_OPT is 0, then opt is False;
  2. otherwise, it's True.

TODOs:

Let me watch how this new CI works!

dlee992 commented 2 months ago

Looks like CI won't automatically run for my commit.

gmarkall commented 2 months ago

Looks like CI won't automatically run for my commit.

It's presently not working, I'm awaiting a fix on the CI.

gmarkall commented 2 months ago

Thanks for the PR - I think the idea is good in principle, so if you'd like to add tests I can test manually if CI isn't working before your PR is ready.

gmarkall commented 2 months ago

/ok to test

dlee992 commented 2 months ago

Looks like this compile function should be also revised as cuda.jit. Quite similar.

@global_compiler_lock
def compile(pyfunc, sig, debug=False, lineinfo=False, device=True,
            fastmath=False, cc=None, opt=True, abi="c", abi_info=None,
            output='ptx'):

Add this to TODOs.

gmarkall commented 2 months ago

Looks like this compile function should be also revised as cuda.jit

Ah, good catch!

dlee992 commented 2 months ago

Suddenly, this is kinda out of control (since _OptLevel is an internal class in config, and have to get its _raw_value field, otherwise need to add a method in that class), but I tried to manage it. need some reviews.

Now, perhaps we need a NUMBA_CUDA_OPT separately, which can simplify code a bit. At the very beginning, I think this should be a one-line change PR : )

gmarkall commented 1 month ago

@dlee992 From your previous comment it sounds like you think there's a problem here, but I can't see what it is - if I review this and it looked good to me, would you be happy with it? (I have already skimmed the PR and can't see an obvious conceptual problem)

dlee992 commented 1 month ago

Yes, please review this.

I just met the same issue when I write tests: https://github.com/numba/numba/issues/9646. And wonder if numba core can have a better way or we can use a separate numba ENVVAR to control this OPT behaviour, but we can discuss it after this PR.

copy-pr-bot[bot] commented 1 month ago

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

gmarkall commented 1 month ago

/ok to test

gmarkall commented 1 month ago

I hope you don't mind that I merged in develop and pushed to this branch - this is because I got the CI setup working again, and wanted to give this a test run.

dlee992 commented 1 month ago

Thanks for clearning up all the "debug with opt" warnings, too. I think there is one more case to clean up in test_raise_in_device_function:

Also added opt=False in this test.

Pushed all requested changes in latest commit.

My local python -m numba.runtests numba.cuda.tests -m passed.

gmarkall commented 1 month ago

/ok to test

gmarkall commented 1 month ago

Thanks for your efforts here! I don't have cudasim working in CI yet, but cudasim tests pass for me locally.