Page MenuHomePhabricator

jdenny (Joel E. Denny)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 2 2017, 3:15 PM (80 w, 4 d)

Recent Activity

Today

jdenny added inline comments to D61643: [PragmaHandler][NFC] Expose `#pragma` location.
Tue, May 21, 7:30 AM · Restricted Project
jdenny added inline comments to D61643: [PragmaHandler][NFC] Expose `#pragma` location.
Tue, May 21, 7:24 AM · Restricted Project

Wed, May 15

jdenny added a comment to D61944: Fixed https://bugs.llvm.org/show_bug.cgi?id=41584.

Thanks, Joel.

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

Wed, May 15, 10:10 AM · Restricted Project
jdenny added a comment to D61944: Fixed https://bugs.llvm.org/show_bug.cgi?id=41584.

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

Wed, May 15, 8:10 AM · Restricted Project

Thu, May 9

jdenny added inline comments to D61467: [Rewrite] Extend to further accept CharSourceRange.
Thu, May 9, 3:59 PM · Restricted Project
jdenny added a comment to D61467: [Rewrite] Extend to further accept CharSourceRange.

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

Thu, May 9, 3:51 PM · Restricted Project

Tue, May 7

jdenny added inline comments to D61643: [PragmaHandler][NFC] Expose `#pragma` location.
Tue, May 7, 10:42 AM · Restricted Project
jdenny added a parent revision for D61509: [OpenMP] Set pragma start loc to `#pragma` loc: D61643: [PragmaHandler][NFC] Expose `#pragma` location.
Tue, May 7, 9:52 AM · Restricted Project
jdenny added a child revision for D61643: [PragmaHandler][NFC] Expose `#pragma` location: D61509: [OpenMP] Set pragma start loc to `#pragma` loc.
Tue, May 7, 9:52 AM · Restricted Project
jdenny updated the diff for D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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

Tue, May 7, 9:46 AM · Restricted Project
jdenny created D61643: [PragmaHandler][NFC] Expose `#pragma` location.
Tue, May 7, 9:41 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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.

Tue, May 7, 9:28 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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.

Tue, May 7, 9:23 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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

Tue, May 7, 9:15 AM · Restricted Project
jdenny updated the diff for D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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.

Tue, May 7, 9:04 AM · Restricted Project
jdenny accepted D61566: Fix for bug 41747: AST Printer doesn't print nested name specifier for out of scope record definitions.

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.

Tue, May 7, 8:13 AM · Restricted Project

Mon, May 6

jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

Thanks for everyone's responses so far.

Mon, May 6, 10:37 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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.

Mon, May 6, 7:56 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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.

Mon, May 6, 6:51 AM · Restricted Project

Sun, May 5

jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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,
Sun, May 5, 4:53 PM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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

Sun, May 5, 8:05 AM · Restricted Project

Fri, May 3

jdenny updated the diff for D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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

Fri, May 3, 10:59 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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

Fri, May 3, 9:09 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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.

Fri, May 3, 8:33 AM · Restricted Project
jdenny added a comment to D61509: [OpenMP] Set pragma start loc to `#pragma` loc.

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

Fri, May 3, 7:52 AM · Restricted Project
jdenny created D61509: [OpenMP] Set pragma start loc to `#pragma` loc.
Fri, May 3, 7:40 AM · Restricted Project

Thu, May 2

jdenny created D61467: [Rewrite] Extend to further accept CharSourceRange.
Thu, May 2, 2:27 PM · Restricted Project
jdenny created D61466: [Rewrite][NFC] Add FIXME about RemoveLineIfEmpty.
Thu, May 2, 2:06 PM · Restricted Project

Tue, Apr 30

jdenny added a comment to D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++.
Tue, Apr 30, 11:42 AM · Restricted Project, Restricted Project, Restricted Project
jdenny added a comment to D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++.

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.

Tue, Apr 30, 9:27 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Apr 26

jdenny added a comment to D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++.

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.

Fri, Apr 26, 7:35 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Apr 23

jdenny committed rL359012: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.
[APSInt][OpenMP] Fix isNegative, etc. for unsigned types
Tue, Apr 23, 10:05 AM
jdenny committed rG3234887fe2ea: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types (authored by jdenny).
[APSInt][OpenMP] Fix isNegative, etc. for unsigned types
Tue, Apr 23, 10:05 AM
jdenny committed rC359012: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.
[APSInt][OpenMP] Fix isNegative, etc. for unsigned types
Tue, Apr 23, 10:05 AM
jdenny closed D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.
Tue, Apr 23, 10:04 AM · Restricted Project, Restricted Project

Mon, Apr 22

jdenny added a comment to D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++.

Does the following match what you have in mind?

Mon, Apr 22, 2:51 PM · Restricted Project, Restricted Project, Restricted Project
jdenny committed rG906b2642517c: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen (authored by jdenny).
[VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen
Mon, Apr 22, 1:25 PM
jdenny committed rL358917: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen.
[VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen
Mon, Apr 22, 1:24 PM
jdenny committed rC358917: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen.
[VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen
Mon, Apr 22, 1:24 PM
jdenny closed D60845: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen.
Mon, Apr 22, 1:24 PM · Restricted Project
jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

Thanks for the reviews! Will push soon.

Mon, Apr 22, 1:13 PM · Restricted Project, Restricted Project

Apr 19 2019

jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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

Apr 19 2019, 4:22 PM · Restricted Project, Restricted Project

Apr 18 2019

jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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.

Apr 18 2019, 5:13 PM · Restricted Project, Restricted Project
jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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?)

Apr 18 2019, 3:18 PM · Restricted Project, Restricted Project
jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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 :)

Apr 18 2019, 3:03 PM · Restricted Project, Restricted Project
jdenny added a comment to D60845: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen.

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.

Apr 18 2019, 2:40 PM · Restricted Project
jdenny added a comment to D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++.

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.

Apr 18 2019, 12:23 PM · Restricted Project, Restricted Project, Restricted Project
jdenny added a comment to D60845: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen.

Thanks for the accepts.

Apr 18 2019, 8:07 AM · Restricted Project
jdenny updated the diff for D60845: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen.

Clarify the behavior of multiple -verify options.

Apr 18 2019, 8:00 AM · Restricted Project

Apr 17 2019

jdenny created D60845: [VerifyDiagnosticConsumer] Document -verify=<prefixes> in doxygen.
Apr 17 2019, 4:42 PM · Restricted Project
jdenny updated the diff for D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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

Apr 17 2019, 1:49 PM · Restricted Project, Restricted Project

Apr 16 2019

jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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?

Apr 16 2019, 8:07 PM · Restricted Project, Restricted Project
jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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.)

Apr 16 2019, 2:33 PM · Restricted Project, Restricted Project
jdenny updated the diff for D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

Duplicate new APSInt test but for signed type.

Apr 16 2019, 1:45 PM · Restricted Project, Restricted Project
jdenny added inline comments to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.
Apr 16 2019, 1:23 PM · Restricted Project, Restricted Project
jdenny added a comment to D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++.

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

Apr 16 2019, 1:07 PM · Restricted Project, Restricted Project, Restricted Project
jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

Ping.

Apr 16 2019, 12:43 PM · Restricted Project, Restricted Project
jdenny added a comment to D60732: [analyzer] NFC: Use -verify=... in MoveChecker tests..

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

Apr 16 2019, 11:50 AM · Restricted Project, Restricted Project
jdenny added a comment to D60732: [analyzer] NFC: Use -verify=... in MoveChecker tests..

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

Apr 16 2019, 11:44 AM · Restricted Project, Restricted Project

Apr 2 2019

jdenny added a comment to D60043: [FileCheck] Fix FileCheck.cpp compilation on Solaris.

LGTM again. Thanks everyone.

Apr 2 2019, 7:23 AM · Restricted Project
jdenny added a comment to D60043: [FileCheck] Fix FileCheck.cpp compilation on Solaris.

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

Apr 2 2019, 6:14 AM · Restricted Project
jdenny added inline comments to D60043: [FileCheck] Fix FileCheck.cpp compilation on Solaris.
Apr 2 2019, 6:00 AM · Restricted Project

Apr 1 2019

jdenny added a comment to D60043: [FileCheck] Fix FileCheck.cpp compilation on Solaris.

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!

Apr 1 2019, 8:50 AM · Restricted Project
jdenny accepted D60043: [FileCheck] Fix FileCheck.cpp compilation on Solaris.

Thanks for the patch.

Apr 1 2019, 8:19 AM · Restricted Project

Mar 29 2019

jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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

Mar 29 2019, 1:33 PM · Restricted Project, Restricted Project
jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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.

Mar 29 2019, 12:34 PM · Restricted Project, Restricted Project

Mar 22 2019

jdenny updated the diff for D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

Extend llvm/unittests/ADT/APSIntTest.cpp.

Mar 22 2019, 1:37 PM · Restricted Project, Restricted Project
jdenny added a comment to D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.

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

Mar 22 2019, 1:05 PM · Restricted Project, Restricted Project
jdenny set the repository for D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types to rG LLVM Github Monorepo.
Mar 22 2019, 12:52 PM · Restricted Project, Restricted Project
jdenny created D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types.
Mar 22 2019, 12:47 PM · Restricted Project, Restricted Project

Mar 15 2019

jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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

Mar 15 2019, 7:47 AM · Restricted Project

Mar 14 2019

Herald added a project to D55269: [CUDA] Fix nvidia-cuda-toolkit detection on Ubuntu: Restricted Project.
In D55269#1320382, @tra 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.

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.

Mar 14 2019, 8:32 AM · Restricted Project

Mar 13 2019

jdenny accepted D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++.

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}.

Mar 13 2019, 10:58 AM · Restricted Project, Restricted Project, Restricted Project

Mar 11 2019

jdenny added a comment to D58784: [FileCheck]Remove assertions that prevent matching an empty string at file start before CHECK-NEXT/SAME.

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.

Mar 11 2019, 8:58 AM · Restricted Project

Mar 8 2019

jdenny added a comment to D58784: [FileCheck]Remove assertions that prevent matching an empty string at file start before CHECK-NEXT/SAME.

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
Mar 8 2019, 12:45 PM · Restricted Project

Mar 1 2019

jdenny added a comment to D58784: [FileCheck]Remove assertions that prevent matching an empty string at file start before CHECK-NEXT/SAME.

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.
Mar 1 2019, 9:56 AM · Restricted Project

Feb 28 2019

jdenny added a comment to D58784: [FileCheck]Remove assertions that prevent matching an empty string at file start before CHECK-NEXT/SAME.

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.

Feb 28 2019, 6:55 PM · Restricted Project
jdenny added a comment to D58784: [FileCheck]Remove assertions that prevent matching an empty string at file start before CHECK-NEXT/SAME.

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

Feb 28 2019, 3:43 PM · Restricted Project

Jan 30 2019

jdenny added a comment to D56789: FileCheck: Allow CHECK-{SAME,NEXT,EMPTY} after initial CHECK-DAG.

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?

Jan 30 2019, 4:06 PM · Restricted Project

Jan 29 2019

jdenny added a comment to D56789: FileCheck: Allow CHECK-{SAME,NEXT,EMPTY} after initial CHECK-DAG.

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.

Jan 29 2019, 10:05 AM · Restricted Project

Jan 26 2019

jdenny added a reviewer for D55725: [OpenMP] Add libs to clang-dedicated directories: jroelofs.

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++.

Jan 26 2019, 7:01 PM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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.

Jan 26 2019, 6:38 PM · Restricted Project

Jan 25 2019

jdenny added a comment to D56789: FileCheck: Allow CHECK-{SAME,NEXT,EMPTY} after initial CHECK-DAG.

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.

Jan 25 2019, 2:40 PM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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.

Jan 25 2019, 1:15 PM · Restricted Project
jdenny updated the diff for D55725: [OpenMP] Add libs to clang-dedicated directories.
  • 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.
Jan 25 2019, 11:15 AM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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.

Jan 25 2019, 10:02 AM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.
  • 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.

Jan 25 2019, 8:42 AM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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.

Jan 25 2019, 8:30 AM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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

Jan 25 2019, 8:02 AM · Restricted Project

Jan 24 2019

jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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.
Jan 24 2019, 2:04 PM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.

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:

Jan 24 2019, 2:00 PM · Restricted Project
jdenny added a comment to D55725: [OpenMP] Add libs to clang-dedicated directories.
Jan 24 2019, 9:33 AM · Restricted Project

Jan 23 2019

jdenny added inline comments to D55725: [OpenMP] Add libs to clang-dedicated directories.
Jan 23 2019, 7:35 PM · Restricted Project
jdenny added a comment to D56789: FileCheck: Allow CHECK-{SAME,NEXT,EMPTY} after initial CHECK-DAG.

Hi Thomas,

Jan 23 2019, 12:36 PM · Restricted Project

Jan 22 2019

jdenny updated the diff for D55725: [OpenMP] Add libs to clang-dedicated directories.

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.

Jan 22 2019, 6:06 PM · Restricted Project
jdenny committed rL351881: [FileCheck] Suppress old -v/-vv diags if dumping input.
[FileCheck] Suppress old -v/-vv diags if dumping input
Jan 22 2019, 1:43 PM
jdenny closed D55825: [FileCheck] Suppress old -v/-vv diags if dumping input.
Jan 22 2019, 1:43 PM

Jan 14 2019

jdenny added inline comments to D55825: [FileCheck] Suppress old -v/-vv diags if dumping input.
Jan 14 2019, 7:35 PM

Jan 10 2019

jdenny added a comment to D56541: [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and `FILECHECK_OPTS` into environment for FileCheck tests..

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

Jan 10 2019, 9:29 AM
jdenny accepted D55940: Detect incorrect FileCheck variable CLI definition.

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.

Jan 10 2019, 8:48 AM