In D61944#1503254, @AndreyChurbanov wrote:

Thanks, Joel.

I actually was not able to reproduce your first problem. But I hope the patch should fix both.

For me, this appears to fix my bug report's second reproducer, the one without the num_teams(3). Thanks!

Ping. This seems like a straight-forward extension to the Rewriter API.

As requested, replace this patch with just the OpenMP changes.

In D61509#1493729, @lebedev.ri wrote:

I don't see why this differential can't be updated to only contain the remaining part
of the diff (for the actual OpenMP change), after splitting the NFC refactoring part.

In D61509#1493718, @lebedev.ri wrote:

In D61509#1493714, @jdenny wrote:

In D61509#1493703, @lebedev.ri wrote:

It would have been better to submit this refactor as a new patch..

Sorry, I didn't realize that was the norm. I can do that now if it would help. I can also revert changes to this patch if the goal is to make it easier to reference the old version.

I think that would be good. This current diff would be a simple NFC cleanup, i think i will signoff it even.

In D61509#1493703, @lebedev.ri wrote:

It would have been better to submit this refactor as a new patch..

As suggested, I've created a struct PragmaIntroducer and I've reduced this patch not to include the OpenMP changes. I'll add a new patch with the OpenMP changes soon.

LGTM except for nits in the tests. I'm not close to C++ support in Clang, so please give other reviewers a few days to comment just in case. I've added a couple who have reviewed my patches in this area in the past.

Thanks for everyone's responses so far.

In D61509#1491898, @ABataev wrote:

Again, I don't see a single point why would we need this. I don't think there is a big difference between the location of pragma keyword and omp keyword.

In D61509#1491537, @lebedev.ri wrote:

In D61509#1491397, @jdenny wrote:

In D61509#1491209, @ABataev wrote:

In D61509#1491204, @lebedev.ri wrote:

In D61509#1491158, @jdenny wrote:

In D61509#1491119, @lebedev.ri wrote:

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

• I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
• This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.
  
  I see two paths forward:
• Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
• Moar abstractions - how about not changing the startloc of openmp directives,

My alternative proposal was exactly that. A difficulty is how to pass the #pragma location to the OpenMP AST node constructors. PragmaOpenMPHandler::HandlePragma passes locations via the tok::annot_pragma_openmp and tok::annot_pragma_openmp_end tokens, so where do we pass this new location? I proposed creating a third token, and Alexey was concerned over the parsing problems that would create.

but instead add some baseclass to `OMPDirective` class (& every other class that is created from pragma), that would record the `PragmaIntroducer`?

I agree that a base class would be a nice way to extend all pragma classes with the PragmaIntroducer.

I'm against this solution. I don't see any reasons why we should do this. Instead, we're getting a lot of pain with parsing and maintenance.

One way to avoid creating an extra token would be to widen the Token class to store the additional location. The Token documentation says it's not intended to be space efficient. How does that sound to people?

That was my proposal, yes.

In D61509#1491209, @ABataev wrote:

In D61509#1491204, @lebedev.ri wrote:

In D61509#1491158, @jdenny wrote:

In D61509#1491119, @lebedev.ri wrote:

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

Sure, I'll work on that. What about NameMe = PragmaIntroducer?

Could work. And then move PragmaIntroducerKind into it.
I believe this part of the refactoring is completely uncontroversial.

For that change, just basing off the clang-tidy diff, neither variant is ideal,

Do you mean that it's better for diagnostics that point to a pragma not to include the #pragma in their locations? If so, why is that?

I'm not sure either one is better than the other one.

I have two concerns:

• I fear this would result in inconsistency with other pragmas, since this will only change openmp-ones. I don't know if it will be accepted to migrate the rest of them in the same way.
• This use-case requires having the location of #pragma, so the entire AST is migrating to store it. But the current location will no longer be accessible from AST.
  
  I see two paths forward:
• Mail cfe-dev, and suggest to do this change for *all* pragmas. Either this is ok for all of them, or none of them.
• Moar abstractions - how about not changing the startloc of openmp directives,

In D61509#1491119, @lebedev.ri wrote:

I recommend to split this into two parts - changing PragmaIntroducerKind Introducer to
struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};, and the actual openmp change.

As discussed, implement OpenMP solution #1 (one-line change in PragmaOpenMPHandler::HandlePragma in ParsePragma.cpp), and update tests.

Thanks for explaining. I'll proceed with solution #1.

In D61509#1489771, @ABataev wrote:

In D61509#1489761, @jdenny wrote:

In D61509#1489752, @ABataev wrote:

If the patch is going to be accepted, then definitely it must be solution #1.

I certainly have no objection to that and will be happy to implement it, but can you help me to understand the rationale? (Thanks for your quick response!)

Another one annotation token may significantly change the parsing process. It will require a lot of rework in the parsing of OpenMP pragmas plus may lead to some unpredictable results like endless parsing in some cases etc.

In D61509#1489752, @ABataev wrote:

If the patch is going to be accepted, then definitely it must be solution #1.

In D59168#1484754, @phosek wrote:

It's the LIBCXX_ABI_VERSION CMake option, see https://github.com/llvm/llvm-project/blob/master/libcxx/CMakeLists.txt#L121

In D59168#1480801, @phosek wrote:

In D59168#1480428, @jdenny wrote:

It seems we have roughly the same situation for their ABIs. That is, for .so files, someone noted that libc++ and libunwind have always used the same version, .1, and openmp doesn't use a version (except in Debian packages). In other words, for each of these three, a change in ABI compatibility has never been indicated. For openmp only, it's apparently assumed a change never will need to be indicated (except in Debian packages).

They already use .2 for ABI v2 which is currently considered unstable but some project like Fuchsia already use. There's also an ongoing discussion to stabilize v2.

In D59168#1479939, @phosek wrote:

In D59168#1474715, @jdenny wrote:

Does the following match what you have in mind?

$prefix/
  include/
    c++/
      v1/
        limits.h
        ...
    openmp/
      v1/ <------ I don't think openmp has anything like this now

Iibc++ uses this scheme to allow the possibility of evolving the API but I'm not sure if it's necessary for openmp.

Does the following match what you have in mind?

Thanks for the reviews! Will push soon.

In D59712#1469392, @lebedev.ri wrote:

Does this pass check-all? check-all of stage-2? test-suite?

In D59712#1472358, @hfinkel wrote:

I've never tried running the other tests you mention, for any patch. I thought people normally left those to the bots. Should this patch be handled differently?

We have a lot of people actively working off of trunk, and we try very hard to keep trunk clean. The bots are a second line of defense, not the primary checkers. In any case, this comes down to professional judgement. It is not uncommon to ask for a patch author to check self hosting and a test suite run before committing - specifically, those patches that might affect correctness, or introduce other subtle problems, and for which running the compiler over a bunch of C/C++ code might uncover a problem.

In D59712#1472342, @lebedev.ri wrote:

In D59712#1472318, @jdenny wrote:

For me, check-all's success is not affected by the current patch. I built all subprojects except llgo, which gave me a build problem independently of this patch.

(Built natively, or with clang with this patch?)

In D59712#1469760, @lebedev.ri wrote:

In D59712#1469693, @jdenny wrote:

In D59712#1469392, @lebedev.ri wrote:

In D59712#1469358, @jdenny wrote:

In D59712#1469301, @lebedev.ri wrote:

In D59712#1469295, @craig.topper wrote:

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

No. The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

Err, i was talking about the current code in the patch :)

In D60845#1472291, @rsmith wrote:

In D60845#1471741, @jdenny wrote:

In D60845#1471002, @rsmith wrote:

I've seen a few projects outside of clang use -verify mode for their own testing of various things.

Interesting. What's an example?

libc++ and absl both use -verify in their test suites.

In D59168#1470578, @phosek wrote:

In D59168#1469186, @jdenny wrote:

I was also thinking about alternative names for the library path, specifically for headers we use include/c++ but for libraries we'll now use lib/clang/<target> which is not very consistent.

Thanks for the accepts.

Clarify the behavior of multiple -verify options.

Add tests for various other fixes caused by this patch, update summary, and add related reviewers.

In D59712#1469392, @lebedev.ri wrote:

In D59712#1469358, @jdenny wrote:

In D59712#1469301, @lebedev.ri wrote:

In D59712#1469295, @craig.topper wrote:

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Does this pass check-all? check-all of stage-2? test-suite?

In D59712#1469301, @lebedev.ri wrote:

In D59712#1469295, @craig.topper wrote:

Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.

Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)

Duplicate new APSInt test but for signed type.

Is there anything I can do to help this patch make progress?

Ping.

In D60732#1469026, @jdenny wrote:

Sometimes you can avoid an explosion of FileCheck prefixes using -D. I'm not aware of anything like -D for -verify.

Just wanted to give a bit more visibility to the underrated technology of writing -verify=a,b,c instead of the #ifdefclutter.

LGTM again. Thanks everyone.

FileCheck.h needs those other non-LLVM headers. This header is different.

In D60043#1449898, @jyknight wrote:

Sounds like this file is missing #include <math.h> (it probably only #include <cmath>). If it wants to use ::log10, it should #include <math.h>. If it wants to use <cmath>, it should use std::log10.

Ideally, this wouldn't cause this sort of error, but see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89855 which I just filed recently about this problem -- although I didn't know it affected llvm!

Why this is broken only in solaris -- in solaris libc, the LIBC math.h header itself defines the overloads for float, and long double in C++ (in addition to the usual C double). But, they do not define the C++11 "any-int-type" overload. Thus, you get the ambiguous overloads resulting from this GCC bug only on solaris. On other platforms, you just see only the double overload. Which in this instance is fine, but if you were passing a float or long double, may not be!

Thanks for the patch.

Thanks. Then the patch just needs someone to review from the ADT side.

Ping. To be clear, my goal here was not to change the OpenMP behavior. My goal was to fix APSInt behavior. If people feel that the old OpenMP behavior is better somehow, we should surely find another way to implement that.

Extend llvm/unittests/ADT/APSIntTest.cpp.

In D59712#1439966, @lebedev.ri wrote:

Might warrant test coverage in llvm/unittest/ADT/APSIntTest.cpp.

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.