I forgot to link this to the RFC we discussed above:

In D55269#1320382, @tra wrote:

In D55269#1320207, @Hahnfeld wrote:

In D55269#1319109, @jdenny wrote:

[...]

In D55269#1318901, @tra wrote:

--cuda-path=/usr was never supposed to work -- /usr is *not* the root of the CUDA SDK.

/usr/lib/cuda/bin/nvcc doesn't exist, so that's probably why FindCUDA.cmake finds /usr/bin/nvcc (also installed by nvidia-cuda-toolkit). Is it fair then to say that /usr/lib/cuda isn't the root either?

[...]

It seems that nvidia-cuda-toolkit still isn't installing a complete CUDA install in one location. Even if it installed it all to /usr/lib/cuda, FindCUDA.cmake would probably still see /usr/bin/nvcc and assume /usr is the CUDA install root.

I think this needs to be fixed then: The shim package should provide links to all necessary things and CMake must be prepared to deal with it. IMO we shouldn't workaround broken build system detection in the compiler.

That's exactly what I proposed to Debian folks https://bugs.llvm.org/show_bug.cgi?id=26966#c6 and I was under impression that that's what they did. It appears that they only created an empty directory structure with version.txt in it. At least that's what I see when I unpack nvidia-cuda-toolkit_9.1.85-3ubuntu1_amd64.deb. Perhaps they do something extra in the install script, but I didn't find anything obvious in the deb itself.

In D55269#1319116, @jdenny wrote:

By the way, nvidia-cuda-toolkit does install the following, but there's no nvvm directory as clang currently expects:

Then again the distro's packaging is broken and needs to be adjusted.

Perhaps we can build a shim package ourselves and distribute it along with the clang. This would decouple usability of clang from the Ubuntu/Debian release process and would make it possible to make clang work with CUDA on older debian/Ubuntu versions.

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 disagree here: It's not OpenMP but CMake that's detecting the wrong path here. This is the place to fix it once and for all (and possibly get the shim package right in that process).

It's somewhat orthogonal, IMO. I agree that it's not OpenMP's problem. Cmake will fix it at some point in future, but, presumably, we want OpenMP buildable *now*. Adding a temporary workaround for something that Cmake can't handle now would make building OpenMP with clang somewhat easier which was the end goal of this patch. In the end it's up to OpenMP maintainers what they would want to do.

In D59168#1423587, @phosek wrote:

I'm not super enthusiastic about the duplicated triple, but the only way to eliminate it would be to move the Clang resource directory inside of lib/clang/x86_64-linux-gnu, i.e. we'd have lib/clang/x86_64-linux-gnu/9.0.0/{include,lib}.

Has anyone raised a solid objection to this patch? If not, I see no reason to delay it further. We can always add replacement assertions later.

In D58784#1422567, @jhenderson wrote:

One question about CHECK-FIRSTLINE - is it required to be the first CHECK in the file? In other words, to check two different things on the first line, would the following be allowed:

CHECK-FIRSTLINE: some stuff
CHECK-FIRSTLINE: more stuff

I don't think it needs to be, because the following would make sense to me:

CHECK-FIRSTLINE: some stuff
CHECK-SAME: more stuff

In D58784#1414631, @probinson wrote:

In D58784#1414574, @jdenny wrote:

Here's how I see it, but I realize it's subjective. If we say that an initial CHECK-SAME matches the first line, then, to be consistent with the relationship CHECK-SAME and CHECK-NEXT have everywhere else, an initial CHECK-NEXT should skip a new line and thus match the second line. That means that, in order to match consecutive lines starting with the first line, we have:

CHECK-SAME: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

That matches my intuition.

But the following seems more intuitive to me:

CHECK-NEXT: first line
CHECK-NEXT: second line
CHECK-NEXT: third line

To say it more abstractly:

• CHECK-NEXT means match on the next line after the previous match.
• CHECK-SAME means match on the same line as the previous match.
  
  If there is no previous match because this is the first directive, then that translates to:
• CHECK-NEXT means match on the next line after no match yet, and that intuitively sounds like the first line to me.
• CHECK-SAME means match on the same line as no match yet, but that sounds impossible to me.

I would say it this way:

• CHECK-NEXT means match on the next line after the starting point
• CHECK-SAME means match on the same line as the starting point
  
  which is more in line with the search-range model.

In D58784#1414493, @probinson wrote:

The following appears to be equivalent to what an initial CHECK-EMPTY should do:

CHECK: {{^}}
CHECK-SAME: {{^$}}

Yep. So if we allow an initial CHECK-SAME, defining the search range as starting at the beginning of the input, then an initial CHECK-SAME: stuff verifies that stuff appears on the first line, and CHECK-SAME: {{^$}} verifies the first line is empty. That seems pretty intuitive to me.

In D58784#1414106, @probinson wrote:

FTR, the CHECK: {{^$}} is ensuring the first line is empty? I see that CHECK-EMPTY isn't allowed as the first directive, unfortunately.

In D56789#1377922, @thopre wrote:

Perhaps we should introduce some sort of only-tested region (e.g. CHECK-ONLY-START and CHECK-ONLY-END with better name) where CHECK-DAG inside would still be able to be reordered but only what is tested by a CHECK-DAG is allowed. It could also allow CHECK directive inside which would behave as CHECK-NEXT. How does that sound?

Or would that CHECK inside behave as a CHECK-SAME? In general, how would whitespace be handled? Error or ignored?

Whatever is consistent with the concept of CHECK-DAG-NEXT I guess. Am suddenly wondering whether 2 CHECK-DAG can match on the same line if they don't overlap?

I've thought about these ideas some more. While ideas like CHECK-NOT-{PUSH,POP} might be more general, I'm inclined to believe that CHECK-DAG-NEXT blocks and CHECK-DAG-SAME blocks could be significantly more user-friendly for the use cases we've been discussing. In any case, before anyone starts implementing one of these ideas, we should probably discuss the details and reach an agreement.

In D55725#1372778, @jdenny wrote:

Assuming installation to that non-version-locked Clang-dedicated directory, one comment there is, "Now if you've built things against those libs, and upgrade your clang version, you are not tied to the new libc++ that comes with it, as you would be with libc++ placed in the resource dir." At first, that sounded nonsensical to me: if every Clang upgrade clobbers the previous Clang's libc++ because the installation directory isn't version-locked, you're always tied to the new libc++.

In D55725#1372480, @Hahnfeld wrote:

I think it's a good idea to have a proper RFC on cfe-dev about how to handle runtime libraries in case they are already installed in system directory. More pointers on historic discussions below.

In D56789#1371821, @thopre wrote:

Fair enough, any clobber can happen between the two, forgot about that. Note that I can see several existing tests in the testsuite that have CHECK-NEXT right after CHECK-DAG which are probably not as strict in terms of testing as their author were expecting, e.g.

test/Analysis/LazyValueAnalysis/lvi-after-jumpthreading.ll
test/Bitcode/thinlto-function-summary-originalnames.ll
test/CodeGen/AArch64/arm64-ccmp.ll
test/CodeGen/AArch64/arm64-indexed-vector-ldst.ll (though it's a single DAG between a CHECK and CHECK-NEXT so it's a special case)
test/CodeGen/AArch64/f16-instructions.ll

and many more.

In D55725#1371349, @jdenny wrote:

In D55725#1370622, @hfinkel wrote:

lib/clang/9.0.0/lib/linux/x86_64 is what this patch currently chooses as it's another place Clang looks. But is that any better or worse than the next option?

So Clang always searches this directory

ToolChain::ToolChain in lib/Driver/ToolChain.cpp always looks for it (it's the last of three directories), so I think that means yes.

but we never put anything there?

Not that I've found in the configurations I've tried.

I vaguely recall that this is where the compiler-rt libs used to be.

I don't know.

• Rename openmp's Clang-dedicated directory from lib/clang/9.0.0/lib/linux/x86_64 to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib because the latter is what other subprojects we've looked at use.

In D55725#1371505, @hfinkel wrote:

Agreed, but it might be worthwhile to choose a guinea pig (openmp) first. I find it difficult to think of all the possible consequences of changes like this.

I feel it's likely there are some things we're not considering, but as a result, the prudent thing to do is to propose the change for libc++ also. We'll, potentially, get a much-wider set of feedback and, based on that, be able to make a better-informed decision. That's the path I prefer that we take. I recommend that you write an RFC for cfe-dev proposing to apply this change to all runtime libraries (openmp, libc++, etc.). It might turn out that there are technical reasons why this would not work for some libraries and would work for others, but I think that we should understand what those reasons are before we proceed. Once we've collected this feedback, the ordering in which we try applying the change to specific projects can be determined. libomp can then be the guinea pig if that's the logical thing to do.

In D55725#1371344, @protze.joachim wrote:

In D55725#1370148, @jdenny wrote:

• libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

From my perspective these names are not for backward compatibility, but to trick GNU and Intel compiler to use the LLVM/OpenMP runtime, when it is found before their own OpenMP runtime. By adding the path of libomp.so to the LIBRARY_PATH, gcc/icc will pick up the LLVM/OpenMP runtime during compilation.

As long as libgomp does not implement OMPT and LLVM has no Fortran frontend, I would welcome the possibility to easily link this runtime into Fortran applications compiled with the GNU compiler.

In D55725#1370823, @Hahnfeld wrote:

In D55725#1369528, @jdenny wrote:

In D55725#1369006, @Hahnfeld wrote:

In the summary you said that you don't want to set -L when compiling which I totally understand, me neither. Did you try setting LIBRARY_PATH in the environment to make Clang automatically add the directory containing libomp.so? That's what we do and the combination of LIBRARY_PATH during compilation and LD_LIBRARY_PATH during execution makes sure that (a) all libraries are found and (b) they are found in the very same version that were used when linking the binary.

LIBRARY_PATH during compilation has no effect for me.

First, -v reveals that -L/home/jdenny/llvm/bin/../lib is before -L/home/jdenny/llvm/lib, which is added by LIBRARY_PATH, which is thus redundant here.

Are you saying a manual -L trumps the environment variable LIBRARY_PATH? Yes, that makes sense, I guess.

In D55725#1370622, @hfinkel wrote:

lib/clang/9.0.0/lib/linux/x86_64 is what this patch currently chooses as it's another place Clang looks. But is that any better or worse than the next option?

So Clang always searches this directory

In D55725#1370148, @jdenny wrote:

I then see the following effects from -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=True:

• Rename the Clang-dedicated directory from lib/clang/9.0.0/lib/linux to lib/clang/9.0.0/x86_64-unknown-linux-gnu/lib.
• For the libclang_rt.*-x86_64.* there, drop the now redundant -x86_64. components from their names.
• Move libc++* and libunwind* from lib to the Clang-dedicated directory.

I'm trying to better understand how various LLVM subprojects currently (without this patch) install to Clang-dedicated library directories. I'm building with the following:

In D55725#1369006, @Hahnfeld wrote:

Hi Thomas,

If this patch is not the right thing to do, or if it conflicts with existing plans for openmp library organization, please let me know. I'd like to understand the best path forward.

In D56541#1352955, @delcypher wrote:

Okay I get the idea. This is a much more invasive change however. Would it be okay to commit this (with FILECHECK_OPTS also stripped out) and do your suggestion as a follow up patch?
The reason I'd prefer to do this is because this change is intended to fix some internal build bots. Due to where we are in the release cycle the changes I make need to be simple (i.e. the smaller the patch the better) in order to be accepted by internal reviewers.

This patch does improve the situation for everyone, and it's easy to revert, so I'm fine with that. But could you fix lit's test suite too? Should be easy, right? I'm not aware of any other test suites that are sensitive to FileCheck's output in this manner.

If it's straight forward I'll land it in a separate patch.

Unless you plan to write the followup patch immediately, could you please add a bugzilla and cc me?

https://bugs.llvm.org/show_bug.cgi?id=40284

The summary needs to be updated for the extraction of D56549. Otherwise, LGTM. I've accepted, but others have time to comment while D56549 is reviewed.

I commented when this was part of D55940. However, I'm not as familiar with this area, especially the unit testing, so someone else should review further.

In D56541#1352784, @delcypher wrote:

In D56541#1352721, @jdenny wrote:

I've been wanting to fix this too. Thanks for working on it. A few issues:

• FILECHECK_OPTS should be cleared too.

Sure I can fix that one.

I've been wanting to fix this too. Thanks for working on it. A few issues:

• In dump-input-annotations.txt, use -implicit-check-not to thoroughly check for trace suppression.
• In dump-input-enable.txt, improve comments, and cover cases of -dump-input=never|fail without -v.

Thanks for reviewing!

Ping.

Thanks for the review!

Update for changes to earlier patches in series.

Update for changes to earlier patch in series.

Split checkConstNotMutableType implementation.

Update for changes to earlier patches in series.

Update for changes to earlier patch in series.

• Reuse new const-not-mutable check when computing predetermined shared attribute.
• Quote 3.1 spec to clarify that private and lastprivate has the const-not-mutable restriction there too.
• Don't add new test case shared_for_const_not_mutable.cpp, which r350127 addressed.

Rebased. Ping.

In D56113#1345529, @jdenny wrote:

In D56113#1345238, @ABataev wrote:

By the way, is there any value to keeping the predetermined shared for const if -openmp-version=3.1 or earlier?

Yes, you can check for the value of LangOpts.OpenMP. For OpenMP 3.1 it will have the value 31.

How far back should we take this? I'm inclined to check for 30 and 31 only and assume anything else is newer, but let me know if we need to check for earlier versions.

I think <= 31 is good. Clang always supported only OpenMP 3.1 and higher.

I'm planning to let this affect the behavior of default(none) (predetermined shared means no explicit attribute is needed).

I don't plan to let it affect which version of the diagnostics are produced. I think the newer diagnostics are clearer even though they are not expressed precisely in terms of 3.1 semantics.

• For OpenMP <= 3.1, add back predetermined shared for const variables without mutable fields.
• Quote the spec for calls to rejectConstNotMutableType for private and lastprivate.
• Update parallel_default_messages.cpp extension now that it depends on the OpenMP version.

In D56113#1345578, @ABataev wrote:

By the way, LangOpts.OpenMP currently defaults to 0. Should it?

Yes, it means it is disabled by default.

Ah, it becomes 1 when we have -fopenmp but not -fopenmp-version. But still LangOpts.OpenMP <= 31 is true by default, so the new behavior would be off by default.

No, by default it gets value 31.

In D56113#1345535, @ABataev wrote:

In D56113#1345529, @jdenny wrote:

In D56113#1345238, @ABataev wrote:

By the way, is there any value to keeping the predetermined shared for const if -openmp-version=3.1 or earlier?

Yes, you can check for the value of LangOpts.OpenMP. For OpenMP 3.1 it will have the value 31.

How far back should we take this? I'm inclined to check for 30 and 31 only and assume anything else is newer, but let me know if we need to check for earlier versions.

I think <= 31 is good. Clang always supported only OpenMP 3.1 and higher.

I'm planning to let this affect the behavior of default(none) (predetermined shared means no explicit attribute is needed).

I don't plan to let it affect which version of the diagnostics are produced. I think the newer diagnostics are clearer even though they are not expressed precisely in terms of 3.1 semantics. Moreover, there are less cases to test this way. Let me know if you think this is wrong. If you want to review the updated patch first, I'll be posting it soon.

It would be good if could keep the original implementation for 3.1.

In D56113#1345238, @ABataev wrote:

By the way, is there any value to keeping the predetermined shared for const if -openmp-version=3.1 or earlier?

Yes, you can check for the value of LangOpts.OpenMP. For OpenMP 3.1 it will have the value 31.

How far back should we take this? I'm inclined to check for 30 and 31 only and assume anything else is newer, but let me know if we need to check for earlier versions.

I think <= 31 is good. Clang always supported only OpenMP 3.1 and higher.

In D56113#1345337, @ABataev wrote:

In D56113#1345238, @ABataev wrote:

In D56113#1345232, @jdenny wrote:

In D56113#1345047, @ABataev wrote:

In D56113#1344210, @jdenny wrote:

In D56113#1342879, @ABataev wrote:

But you will need another serie of patches for reduction and linear clauses to update their error messages.

Those already had their own checks for const. I'm not aware of any cases not handled, and the test suite does pass after my patch.

Yes, they have the checks for constness, but you need to update those checks to emit this new error message for const values with mutable fields.

I believe you mean we should reuse rejectConstNotMutableType for reduction and linear rather than leave some of its implementation duplicated for those.

For reductions, this will change the message from "const-qualified list item cannot be reduction" to "const-qualified variable cannot be reduction". Is that OK? (There are many tests to update, so I want to ask first.)

Mmmm. For the reductions better to keep the original message, because the list items also might be the array sections, not only the variables.

In D56113#1345238, @ABataev wrote:

By the way, is there any value to keeping the predetermined shared for const if -openmp-version=3.1 or earlier?

Yes, you can check for the value of LangOpts.OpenMP. For OpenMP 3.1 it will have the value 31.

How far back should we take this? I'm inclined to check for 30 and 31 only and assume anything else is newer, but let me know if we need to check for earlier versions.

I think <= 31 is good. Clang always supported only OpenMP 3.1 and higher.

In D56113#1345047, @ABataev wrote:

In D56113#1344210, @jdenny wrote:

In D56113#1342879, @ABataev wrote:

But you will need another serie of patches for reduction and linear clauses to update their error messages.

Those already had their own checks for const. I'm not aware of any cases not handled, and the test suite does pass after my patch.

Yes, they have the checks for constness, but you need to update those checks to emit this new error message for const values with mutable fields.

Other than nits I've added as inline comments, LGTM. However, let's give Paul some time to respond now that the holidays are over.

Ping?

In D56113#1342879, @ABataev wrote:

But you will need another serie of patches for reduction and linear clauses to update their error messages.

• Apply reviewer suggestions.
• Add test case for change in default(none) behavior.

Hi Alexey,

In D56113#1341940, @ABataev wrote:

The patch does not seem quite correct. I committed another fix for this problem.

In D55940#1338920, @thopre wrote:

I've just given it a try but there seems to be some sorcery involved in that case: G contains just 10 instead of =10. Any way I could prevent the parsing from eating a leading equal sign? If not, I could reword the error message to mention leading equal sign but we would fail to diagnose an issue when passing -D'=Expression: VAR-5=10' typed by error instead of -D'TXT=Expression: VAR-5=10'.

In D55940#1337906, @probinson wrote:

This will permit -DVALUE= (with no value) is that a supported usage?

Better diagnostics would definitely be helpful here.

Apply variable renames suggested by grokos.

• Fix a comment typo.