Fixed a typo.

Nice. Thank you for adding the tests.
LGTM.

The tests you have only check the end result and do not directly verify the new functionality the patch adds.

You probably want to remove libcalls.patch from the patch diff.

In D55145#1331301, @denzp wrote:

In D55145#1324230, @tra wrote:

Here the behaviour of unsupported instructions is changed to match the behaviour of explicit intrinsics calls.

If I remember correctly, the crash was only happening for instructions lowered to libcall. Do we crash in any other case?
If libcall is the only source of failure, would it make more sense to bail out of this function early? We're going to fail in this case anyways.

You are right, normally it would make sense. But in this case, the end goal is to support these libcalls.
It would be easier to achieve when both instructions and intrinsics are behaving consistently. And I'm afraid, bailing them out, would be an extra work that will have to be dropped when the final libcall solution will come (means, very-very soon).

jlebar@ LGTM'ed via email.
Landed in rL348662

Here the behaviour of unsupported instructions is changed to match the behaviour of explicit intrinsics calls.

LGTM.

In D55269#1320207, @Hahnfeld wrote:

I think there are some misunderstandings here, or at least I understand things differently than @tra is describing them: AFAICS this change is NOT about replacing nvcc by clang in any CMake project (be that the OpenMP runtime or any other project outside of LLVM).

In D55269#1319437, @jdenny wrote:

In D55269#1319382, @tra wrote:

Let's start with fixing OpenMP's cmake files. Once it no longer insists on specifying --cuda-path=/usr, and isUbuntu is in place, what is the remaining failure that you see?

I'm fairly certain I would see no remaining failure then, but it's not clear to me how we should convince OpenMP's cmake files to stop doing that.

openmp/libomptarget/cmake/Modules/LibomptargetNVPTXBitcodeLibrary.cmake is the file, and there's one occurrence of --cuda-path.

If CUDA_TOOLKIT_ROOT_DIR is specified explicitly to cmake, that --cuda-path is necessary, so we cannot just remove it.

If CUDA_TOOLKIT_ROOT_DIR is not specified explicitly, it is computed by cmake. So I believe this boils down to getting cmake to find the right CUDA root on Debian/Ubuntu.

Do you agree so far?

In D55269#1319319, @jdenny wrote:

My real goal is to get clang and openmp working out of the box on Ubuntu. Treating --cuda-path=/usr as a special case was just a way to get there. It's odd apparently because nvidia-cuda-toolkit is odd.

It appears that what you're trying to do is to add "/usr/lib/cuda" on Ubuntu and Debian when --cuda-path=/usr is specified.
This is a rather odd thing to do. In the end only one of those paths will be in effect and that's the path that should be specified via --cuda-path. The fact that you want to add /usr/cuda/lib in this case suggests that /usr is the wrong path and /usr/lib/cuda is the correct one. It sounds like you need to change your build system and tell clang the correct path.

I'm not sure that's something that needs to be fixed in clang.

Do I understand it correctly that the bug appears to affect HIP compilation only?

I'm not convinced that nvcc's behavior is a good guide for clang's defaults.

This particular change is largely cosmetic. I've just spotted this nit while I was debugging a different problem.

LGTM.

Landed in rC342924

It's unfortunate that lit does not scrub the environment in order to make tests hermetic.

Great to see someone beating me to add support for a new CUDA version. :-)
I've posted my patch in D52438. It's very similar to yours with a couple of other necessary changes.

Overall the patch look OK. I'll take a closer look on Monday.

Added '>=' and '>>=' to the list of tokens that may indicate the end of the
empty template argument list.

The patch does not seem to match the description and appears to have nothing to do with rearranging include paths. Could you check if these are the changes you intended to send for review.

Renamed last instance of 'Matches' -> 'PreventedBy'.

Updated assertion message.

• Check that D is non-null

In D52179#1237194, @JonasToth wrote:

Is the condition for this assertion checked beforehand or could this create runtime failures?

Addressed Richard's comments.
Moved clang-tidy changes into separate review D52179.

@rsmith ping.

@jlebar Justin, can you take a look?

In D51663#1224057, @MatzeB wrote:

LGTM, sorry for the breakage.

Updated the changes according to Matthias' comments.

How about D51663?

Your patch above *almost* works, except that each test variant wants to create the same symlink $BUILD/ExternalCUDA/$TEST.teferece_output -> $SRC/External/CUDA/$TEST.reference_output. If symlinks are created at different points in time ninja manages to avoid conflicts, but typically I get one or two attempts to create the symlink launches simultaneously and one of them fails with "symlink already exists". Perhaps the symlink target name should be uniquified in some way.

In D51048#1221432, @MatzeB wrote:

Though thinking about it, I don't really understand your problem:

How can there be multiple outputs with the same name? They have different names in the repository (algorithm.reference_output, assert.reference_output, ...)

Not needed anymore after the reverts in rC341115 and rC341118, right?