Ack.

I've discussed this with @echristo . Bottom line is that this patch is OK to proceed, and that I'm on the hook to add a NVPTX-specific pass to enforce that all target-cpu and and target-feature attributes in the TU are compatible with the current compilation parameters. I.e. if we're compiling for sm_50, then IR with functions that have target-cpu=sm_60 will result in an error.

Few nits. LGTM syntax-wise. I'm not very familiar with backend, so will defer to someone who has better idea.

Last comment in the bug pointed out that those overloads should be constexpr in c++14. Maybe in a separate patch, though.

Thank you.

In D47691#1123513, @hfinkel wrote:

Bottom line -- the situation is far from perfect, but IMO the patch does sensible thing if we're compiling IR with mixed target-cpu and target-features attributes using NVPTX.

In summary, I think that's okay so long as there aren't intrinsics that depend on target features.

Drive-by review:

In D47849#1124638, @Hahnfeld wrote:

IMO this goes into the right direction, we should use the fast implementation in libdevice. If LLVM doesn't lower these calls in the NVPTX backend, I think it's ok to use header wrappers as CUDA already does.

Few nits. LGTM overall.

Few minor nits/suggestions. LGTM otherwise.

@rsmith - Richard, can you take a look?

Just to make it clear, I'm not against making a sensible default choice, but rather want to make sure that it is possible to override it if the user needs to.

I'm not sure this is the right thing to do. What if user explicitly wants device-side dependencies and runs clang --cuda-device-only -M ? This patch makes it impossible.

I'll wait to see if that fixes @Hahnfeld's problem.

I've experimented a bit and I think that we may not need this patch at all.
As far as I can tell, nv_weak is only applicable to device functions. It's ignored for global kernels and is apparently forbidden for data.
For device functions nvcc produces .weak attribute in PTX.

With the updated patch description + the discussion I'm OK with the approach from the general "how do we compile/use CUDA" point of view. I'll leave the question of whether the approach works for OpenMP to someone more familiar with it.

Looks OK overall, but I'm not very familiar with CGCall, so you may want to dig through the history and find someone with more expertise.

IMO overriding TargetTransformInfo::areInlineCompatible to always return true on NVPTX is what we want to do instead of upgrading everything else.
AFAICT, on NVPTX there's no reason to prevent inlining due to those attributes -- we'll never generate code, nor will we ever execute it on any other GPU than we're currently compiling for.

IIUIC, nv_weak is a synonym for weak (<rant>why, oh why did they need it?</rant>)
You may need to hunt down and change few other places that deal with the weak attribute.
E.g.: https://github.com/llvm-project/llvm-project-20170507/blob/master/clang/lib/AST/Decl.cpp#L4267
https://github.com/llvm-project/llvm-project-20170507/blob/master/clang/lib/CodeGen/ItaniumCXXABI.cpp#L3045

In D47394#1118223, @gtbercea wrote:

I tried this example (https://devblogs.nvidia.com/separate-compilation-linking-cuda-device-code/). It worked with NVCC but not with clang++. I can produce the main.o particle.o and v.o objects as relocatable (-fcuda-rdc) but the final step fails with a missing reference error.

In D47432#1116186, @nhaehnle wrote:

I agree on the test case, unfortunately it requires a lot of setup to make the backend do anything useful at all.

Perhaps unrelated documentation cleanup can be extracted into a separate patch.

LGTM in general.
It would be good to add a test that we can use bits w/o explicit casting now.

One nit. LGTM otherwise.

"Interoperability with other compilers" is probably a statement that's a bit too strong. At best it's kind of compatible with CUDA tools and I don't think it's feasible for other compilers. I.e. it will be useless for AMD GPUs and whatever compiler they use.

In D47396#1113016, @hfinkel wrote:

Does this just not work well with other linkers, do other linkers also have special handling, or does it work better elsewhere for some other reason?

One small nit. LGTM otherwise.

CUDA does not expose explicit AS on clang size. All pointers are treated as generic and we infer specific address space only in LLVM.
__nvvm_atom_*_[sg]_* builtins should probably be removed as they are indeed useless without pointers with explicit AS and NVCC itself does not have such builtins either. Instead, we should convert the generic AS builtin to address-space specific instruction somewhere in LLVM.

In D47070#1106018, @echristo wrote:

As a short-term fix we can disable feature-to-function attribute propagation for NVPTX until we fix it.

@echristo -- any other suggestions?

This is some of what I was talking about when I was mentioning how function attributes and the targets work. Ideally you'll have a compatible set of features and it won't really cause an issue. The idea is that if you're compiling for a minimum ptx feature of X, then any "compatible" set of ptx should be able to inline into your code. I think you do want the features to propagate in general, just specific use cases may not care one way or another - that said, for those use cases you're probably just compiling everything with the same feature anyhow.

@sanjoy is more familiar with this, so I've asked him to take a look.

One more thing -- it would be really good to add some tests to make sure your commands are constructed the way you want.

Sorry about the long silence. I'm back to continue the reviews. I'll handle what I can today and will continue with the rest on Tuesday.

This was not intended. :-( I was unaware that GetCPUAndFeaturesAttributes() would add any feature that looks like a valid CPU name to the target-cpu attribute.
All I needed is to make builtins available or not. Setting them as function attributes is not what we need here.

In D46782#1096648, @rsmith wrote:

Rather than suppressing the warning, should we instead give such variables external linkage?

Fixed mangled comment.

Small nit. LGTM otherwise.

Great! Let's close this review then.
And good luck with cling.

Perhaps we should take a step back and consider whether this is the right approach to solve your problem.

In D46147#1081990, @arsenm wrote:

In D46147#1081951, @bkramer wrote:

Would it be possible to get rid of the flag entirely and make this controlled only by the data layout? That's defined on the module level.

Yes, ultimately the switch is just changing the datalayout. If this is just a short term thing for migration I think a global flag is fine. If you want to be able to swap these properly long term, this is a different arch name / TargetMachine.

Do not use subtarget feature to control codegen of short pointers.
Instead we need to add a field to TargetOptions which is the only way to pass this info to NVPTXTargetInfo constructor where we calculate DataLayout.

@arsenm Will this do?

Moved control of the feature from subtarget feature to --nvptx-short-ptr command-line option.

In D46147#1081662, @arsenm wrote:

The subtarget is a function level property. The pointer size / datalayout is a module property and can't be different between functions. If you have multiple functions with different pointer sizes in a module (or no functions at all), then what pointer size ends up used for a global? The global itself has a size, and any ConstantExpr pointers in its initializer will also need to have a size.

In D46147#1081632, @arsenm wrote:

NVPTX wasn't already doing this? As this is a module property, a subtarget feature can't be used for this. For example what happens for a global value?

Use DataLayout to obtain pointer size.

Updated feature description.

UseShortPointer -> UseShortPointers

Removed debug printout.

Lgtm.