[libcxx-commits] [libcxx] [libc++] Protect the libc++ implementation from CUDA SDK's `__noinline__` macro (PR #73838)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> @philnik777 Nobody is overruling you. I re-requested your review. I appreciate you taking the time to spell out your concerns.

Saying this can be merged while others having concerns/alternative ways forward that aren't addressed does feel like overruling to me.

> > I don't see any reason fur us to make our implementation work super well with cude if there doesn't seem to be much incentive from nvidia to make things work well for other libraries.
> 
> I would say the incentive for us is to help our users, who have asked us for this.

Users have asked that our code works with cuda, which is the case with my proposed alternative, so I don't see how this doesn't address that.

> > I'm also not convinced that this parallels with other macros we push and pop, since this isn't about conformance.
> 
> Could you say more? For me the parallel is "Expansion of this macro breaks libc++".
> 
> Fundamentally this change simply adds a name to a list. I don't share your concern regarding comparative complexity.
> 
> My concern is shipping an implementation where `_LIBCPP_NOINLINE` is a lie when it doesn't need to be. I believe having source code behave as written is a better outcome than having the compiler silently swallow the diagnostic.

I don't think it doesn't behave as written. `noinline` is basically a hint that a function shouldn't be inlined. Not adding it doesn't break anything.

> You've requested changes on this CL. Could you please reiterate the actionable items you would like taken?

I would like to explore simply using a different spelling to avoid the hard error.




> > @ldionne asked for an issue filed at the CUDA project. This has not been done.
> 
> Above in the thread @Artem-B has linked to a bug about this that was filed against the Nvidia Thrust library, and was diagnosed by an Nvidia employee as "This issue is unique to clang" and closed: [NVIDIA/thrust#1703](https://github.com/NVIDIA/thrust/issues/1703)

This bug has been filed against thrust, but the actual problem is the cuda sdk IIUC. I'm not aware of any bug filed against that.

https://github.com/llvm/llvm-project/pull/73838