Syntax/style looks OK to me with a few nits.

In D128752#3616837, @jhuber6 wrote:

In D128752#3616831, @tra wrote:

Do we have tests that verify -target-feature arguments? It may be worth adding a test case there checking for redundant features.

Yeah, we have some existing tests that check for including the target features at least once. I felt like there was no need to include a test for what was more or less an oversight

Do we have tests that verify -target-feature arguments? It may be worth adding a test case there checking for redundant features.

In D128752#3616579, @jhuber6 wrote:

In D128752#3616553, @tra wrote:

we no longer will have a cached CUDA installation so we will usually create it twice.

Does that result in extra output in case we find an unexpected CUDA version, or when compiler is run with -v ?

We may want to wrap installation detector creation into some sort of singleton creation function.

We already create another one for the call coming from Clang, this patch just gets rid of the other call. I think the printouts you're talking about come from the variable in the CUDA toolchain specifically. Here we simply create one to get the version and throw it away. It's not ideal to do the same work twice, so we could wrap this into some singleton interface. Maybe a static optional value inside of the Toolchains/Cuda.cpp file wither a getter that returns a reference to it. Though I don't think this is likely to be a bottleneck.

we no longer will have a cached CUDA installation so we will usually create it twice.

@nikic -- we could use your input here. The patch looks OK to me, but I'm not particularly familiar with AA machinery in general and could use a second opinion.

The linker wrapper cannot do anything with these embedded PTX files because we do not know how to link them,

In D127901#3603118, @jhuber6 wrote:

In D127901#3603006, @tra wrote:

Then we do need a knob controlling whether we do want to embed PTX or not. The default should be "off" IMO.
We currently have --[no-]cuda-include-ptx= we may reuse for that purpose.

We could definitely re-use that. It's another option that probably need to go inside the binary itself since normally those options aren't passed to the linker.

LGTM with a nit.

In D127901#3602771, @jdoerfert wrote:

Do we want/need PTX, I do not, but I don't mind having it. Someone will ask for it eventually.

Very nice. LGTM.

In D127948#3600082, @nhaehnle wrote:

Thank you. One more thought after sleeping on it: What do you think about !exists? I think !exists<T>("foo") reads nicely as "does there exist a record of type T with name 'foo'?". It's not perfect, but I like that there's less confusion with "instanceof" or "isinstance" operators that some programming languages have.

Good catch! Thank you for fixing this.

Playing devil's advocate, I've got to ask -- do we even want to support JIT?

LGTM in general.

In D127739#3584132, @pcwang-thead wrote:

I have considered adding a new operator !instanceof<T>(s) to check if whether a record with type T and name s exists, but I think !isa have the same semantic meaning with !instanceof,

AFAICT, the test case you've added works fine with the compiler at HEAD: https://cuda.godbolt.org/z/q3xYMfdeb
I guess it only shows up in assertion-enabled builds. Can you check what happens if you run the test case compiled as CUDA? I suspect it will have the same issue -- we don't override setAuxTarget at all.

Do you have a practical use case where this feature would be needed?

Good catch. Thank you for fixing this.

LGTM overall with a couple of nits.

I think the root cause of the problem here is that CUDA compilation assumes that the GPU-side looks identical to the host side of the compilation. The test was intended to verify that and, AFAICT, it did its job flagging the issue here.

Looks OK syntax-wise. Library path should probably be fixed, though it appears to be a somewhat separate issue and could be done in its own patch, if the required change is not trivial.

@jhuber6 -- @MaskRay has found that ninja install is failing in a clean build with:

clang: error: unable to execute command: Executable "clang-offload-packager" doesn't exist!

This patch only supports printing these sections, later we will want to
support dumping files the user may be interested in via another flag. I
am unsure if this should go here in llvm-objdump or llvm-objcopy.

LGTM in principle, with new nits.

In D125904#3538742, @jhuber6 wrote:

In D125904#3538714, @tra wrote:

It would be great to have some compile-time checks for that, if possible. Otherwise it will only manifest at run-time and the end user will have no clue what's going on.

Not sure how we could check it at compile-time, if we knew what it was supposed to be we could just set it properly right?

In D125904#3538163, @yaxunl wrote:

I am OK with this patch.

How much work would it take to add cuid generation in the new driver, similar to what the old driver does, using the same logic, however imperfect it is? I'd be OK with that as a possibly permanent solution.

Currently, we infer the usage of the device linker by the user
specifying an offloading toolchain, e.g. (--offload-arch=...) or
(-fopenmp-targets=...), but this shouldn't be strictly necessary.

Is this patch in its current form blocking any of your other work? no-cuid approach, even if we figure out how to do it, will likely take some time. Do you need an interim solution until then?

LGTM. Do you need me to land it?

I'm still itching to figure out a way to avoid CUID altogether and with the new driver it may be possible.
CUID serves two purposes:
a) avoid name conflicts during device-side linking ("must be globally unique" part)
b) allow host to refer to something in the GPU executable ("stable within TU" part)

I would suggest separating it into separate LLVM and MLIR patches.

Couple of nits. LGTM otherwise.

In D125904#3532545, @jhuber6 wrote:

clang a.c -c -o a-0.o Has some externalized static variable.
clang a.c -c -o a-1.o
clang a-0.o a-1.o Redefined symbol error

In D126226#3532471, @jhuber6 wrote:

In D126226#3532423, @tra wrote:

We keep running into the same old underlying issue that we do not have a good way to name/reference specific parts of the compilation pipeline. -Xfoo used to work OK for the linear 'standard' compilation pipeline, but these days when compilation grew from a simple linear pipe it's no longer adequate and we need to extend it.

Yeah, it's getting increasingly complicated to refer to certain portions of the compilation toolchain as we start adding more complicated stuff. Just recently I had a problem that I wanted to pass an -Xclang argument only to the CUDA toolchain, and there's no way to do it as far as I can tell. It may be worth revisiting this whole concept to support more arbitrary combinations.

The one downside to this is that we are no longer stable under multiple compilations of the same file.

In D126226#3532257, @MaskRay wrote:

It's better to avoid JoinedAndSeparate for new options. It is for --xxx val and --xxxval but not intended for the option this patch will add.

In D126226#3532147, @jhuber6 wrote:

We already use this approach for the -Xopenmp-target=<triple> <arg> option that forwards arguments to the given toolchain, so that's what I was using as a basis.

The syntax is confusing. Normally only triple would be the argument for -Xoffload-linker option.
Having both -Xoffload-linker and -Xoffload-linker= variants also looks odd to me.

Looks like it's a wash -- public API is smaller, but the implementations didn't really benefit much. If anything, they got a bit less readable.

My original suggestion which resulted in this patch was to generalize the API to just one function accepting Instruction* and let the target deal with the details.
For many targets there's a fair amount of duplicated code across the per-instruction variant implementation of shouldExpandX and using single call would allow to consolidate the atomic expansion logic in fewer places.

LGTM with a possible nit.

@echristo - FYI, in case you have an opinion on target builtin feature checks.

My concerns have been addressed. I'll defer the final LGTM to @Anastasia.

What's the minimum SM version do we currently support? I did find from one of your patch (https://reviews.llvm.org/D24943) that it had the function hasAtomAddF32, but now I can't find it. Was it removed because we support a minimum SM version, like SM20?

Oops. forgot to hit 'submit' last week, so there's some overlap with Aaron's question.

Tests?

Good catch. LGTM.

Please add a TODO around the items that need further work, so we don't forget about them. Review comments tend to fade from memory.

OK. LGTM.

LGTM in principle.

In D124866#3501641, @yaxunl wrote:

If we are to add __forceinline__ as a keyword, I feel it better be a separate patch to be cleaner.