This is an archive of the discontinued LLVM Phabricator instance.

Read path to CUDA from env. variable CUDA_PATH on Windows
AcceptedPublic

Authored by mojca on Nov 25 2021, 8:11 AM.

Details

Reviewers
tra
Hahnfeld
Summary

This is heavily related to https://reviews.llvm.org/D114326 and uses a different approach for locating path to CUDA on Windows.

Diff Detail

Unit TestsFailed

Event Timeline

mojca created this revision.Nov 25 2021, 8:11 AM
mojca requested review of this revision.Nov 25 2021, 8:11 AM
mojca added inline comments.
clang/lib/Driver/ToolChains/Cuda.cpp
137–141

Sorry, wrong patch. I'll upload it again.

mojca updated this revision to Diff 389808.Nov 25 2021, 8:25 AM
mojca updated this revision to Diff 389811.Nov 25 2021, 8:27 AM
tra added inline comments.Nov 29 2021, 11:29 AM
clang/lib/Driver/ToolChains/Cuda.cpp
137

Do we want to keep this as the fall-back for cases when CUDA_PATH is not set?

Otherwise, we're risking to break existing users.

mojca added inline comments.Nov 29 2021, 12:06 PM
clang/lib/Driver/ToolChains/Cuda.cpp
137

That was going to be my question as well (for which I wanted your input).
From the initial comments in the other review (D114326) I understood that eventually you wanted to get rid of the hardcoded list of versions (but I didn't fully understand the intent).

(Technically speaking this won't break existing users since they were only able to access CUDA versions up to 8.0 until now, and we would just remove support for anything older that CUDA 10.0.)

tra added inline comments.Nov 29 2021, 1:20 PM
clang/lib/Driver/ToolChains/Cuda.cpp
137

Good point. Then it's a good opportunity to decommission the auto-search by version magic and replace it with something more predictable.

I think this patch would obviate the D114326, too, and we should be able to remove the multi-version search everywhere.

tra accepted this revision.Nov 29 2021, 1:20 PM
This revision is now accepted and ready to land.Nov 29 2021, 1:20 PM
mojca added a comment.Dec 5 2021, 9:15 AM

What can/should I do next in order to proceed with this?

tra added a comment.Dec 6 2021, 11:20 AM

What can/should I do next in order to proceed with this?

Next step would be to commit the patch. If you do not have commit access, I can help you with that.
If that's the case, please let me know the name and email you want the commit to be attributed to.

tra added a comment.Jan 5 2022, 11:53 AM

Ping. @mojca, do you need help landing the patch?

mojca added a comment.Jan 6 2022, 12:22 AM

Ping. @mojca, do you need help landing the patch?

Yes, please. I don't have commit access yet.
You can attribute it to mojca at macports.org, for example.

We also need a fix for unit tests on the CI in order to find the files, but I cannot help with that as I don't know how that works.

mojca added a comment.Jan 6 2022, 12:28 AM

Also, I would like to get to do some further "baby steps" towards better support of CUDA on Windows in particular, but I would need some guidelines.
I requested a special channel that would allow a bit of discussion https://discord.com/channels/636084430946959380/.
What would be the best way to shortly discuss the options?

tra added a comment.Jan 6 2022, 12:00 PM

Ping. @mojca, do you need help landing the patch?

Yes, please. I don't have commit access yet.
You can attribute it to mojca at macports.org, for example.

Will do once we sort out the tests.

We also need a fix for unit tests on the CI in order to find the files, but I cannot help with that as I don't know how that works.

You may need to adjust clang/test/Driver/cuda-windows.cu and possibly the mock CUDA installation under clang/test/Driver/Inputs/CUDA-windows.

Also, I would like to get to do some further "baby steps" towards better support of CUDA on Windows in particular, but I would need some guidelines.
I requested a special channel that would allow a bit of discussion https://discord.com/channels/636084430946959380/.
What would be the best way to shortly discuss the options?

Email would work. cfe-dev@ list would work, too. Chat does not work well for me -- my reply latency tends to be higher than that of email and it's not as good for keeping a record of the conversation.

Herald added a project: Restricted Project. · View Herald TranscriptJan 2 2023, 3:13 AM
Herald added subscribers: mattd, MaskRay. · View Herald Transcript