-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
A few fixes for compiling with nvc++ #14139
Conversation
The previous code causes a linker failure due to a duplicated definition of `PointerLikeTypeTraits<decltype(PointerUnion<PTs...>::Val)>::NumLowBitsAvailable`.
Can one of the admins verify this patch? |
@phsft-bot build |
Starting build on |
@@ -261,8 +261,9 @@ struct PointerLikeTypeTraits<PointerUnion<PTs...>> { | |||
|
|||
// The number of bits available are the min of the pointer types minus the | |||
// bits needed for the discriminator. | |||
static constexpr int NumLowBitsAvailable = PointerLikeTypeTraits<decltype( | |||
PointerUnion<PTs...>::Val)>::NumLowBitsAvailable; | |||
using nvhpcWorkAround = decltype(PointerUnion<PTs...>::Val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this patch a backport of the llvm upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, LLVM upstream (17.0.6) does not have this change and nvc++ fails equally there. What is the procedure to apply such a workaround to ROOT's LLVM?
(while NVIDIA may be working on a fix to nvc++)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be enough if we open a PR against LLVM mainline and backport it here. It'd be better to wait until it gets merged upstream but generally there is no need to.
cc: @hahnjo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, I'm not a fan of introducing a workaround to our copy of LLVM for a very specific compiler. I believe upstream has a very clear documentation on which compilers it supports, and NVIDIA's isn't one of them. It's not our job to fix this downstream.
Alright, this gets more tricky than anticipated. I had to workaround a few more codegen/optimizer bugs in nvc++. I changed the PR to draft for now. |
01bca56
to
f330769
Compare
I propose to close this: As I expressed above, I believe it's not our job to fix upstream LLVM to compile with |
Fine for me, I will open a separate PR with the reasonable changes. |
This Pull request:
Changes or fixes:
Fixes several issues when compiling ROOT with nvc++, but not all of them. The build still fails ultimately, because
clang-tblgen
crashes. See also #9036.Checklist: