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

clang: Disable unused-function warnings for 3rd-party NCCL library #599

Merged
merged 2 commits into from
Feb 15, 2020

Conversation

kpu
Copy link
Member

@kpu kpu commented Feb 11, 2020

Actually a lot of warnings from clang are unused functions inside NCCL. Not all of them, but it's a start.

@kpu kpu requested a review from frankseide February 11, 2020 20:12
@kpu
Copy link
Member Author

kpu commented Feb 15, 2020

If you're going to express opinions, at least act on them.

"I am not a fan of warnings. I would request that all warnings in our own code are resolved, while it is OK to disable warnings in 3rd-party code after checking them once." --@frankseide
"We should not fix warnings in third-party code. For that, we should instead configure the compiler to not warn." --@frankseide
"As Frank said, either by addressing the issue, or if 3rd-party code or compilers being unreasonable by disabling them. Disabling has to be well motivated though." --@emjotde
"I would be OK to accept this PR (assuming all other comments are addressed), and working on clang on Linux in parallel, if you guys could commit to submit a second PR that fixes any remaining Mac-specific warnings after the clang on Linux has been made warning-free. Would that work for you (Nick, Kenneth, Marcin)?" --@frankseide

@emjotde
Copy link
Member

emjotde commented Feb 15, 2020

Relax.

@frankseide
Copy link
Contributor

Sorry, did not realize that this was meant for me. Looking now.

@kpu kpu merged commit 63272d1 into master Feb 15, 2020
@kpu
Copy link
Member Author

kpu commented Feb 15, 2020

Thanks @frankseide

emjotde added a commit that referenced this pull request Feb 15, 2020
@emjotde
Copy link
Member

emjotde commented Feb 15, 2020

Reverting for now as this seems to break NCCL compilation with CUDA with g++

@emjotde
Copy link
Member

emjotde commented Feb 15, 2020

Also confirmed on my local machine, I think the CXXFLAGS overwrite is removing some options?

@kpu
Copy link
Member Author

kpu commented Feb 15, 2020

Feel free to revert; I'll try again on that machine. Funny thing is I tried passing CXXFLAGS through too and that's still broken??

@emjotde
Copy link
Member

emjotde commented Feb 15, 2020

Could it be it's removing options from the Make? Like Fpic?

@kpu
Copy link
Member Author

kpu commented Feb 15, 2020

@kpu
Copy link
Member Author

kpu commented Feb 15, 2020

Actually you just need to update your NCCL submodule: https://github.com/NVIDIA/nccl/blob/master/makefiles/common.mk

@emjotde
Copy link
Member

emjotde commented Feb 15, 2020

Downgraded on purpose, higher version have a bug for models above 2GB, unfortunately. Mentioned this in Rikhards issue.

We may need to make a special branch in our fork?

@kpu
Copy link
Member Author

kpu commented Feb 15, 2020

Ugh yeah I just re-read #597. Is a special branch of NCCL overkill for compiler warnings on clang?

@snukky snukky deleted the nccl-unused-function branch February 15, 2022 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants