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 (71 w, 6 d)

Recent Activity

Fri, Mar 15

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

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

Fri, Mar 15, 7:47 AM · Restricted Project

Thu, Mar 14

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.

Thu, Mar 14, 8:32 AM · Restricted Project

Wed, Mar 13

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

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

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

Mon, Mar 11

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.

Mon, Mar 11, 8:58 AM · Restricted Project

Fri, Mar 8

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

Fri, Mar 1

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

Thu, Feb 28

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.

Thu, Feb 28, 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.

Thu, Feb 28, 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
jdenny added a comment to D56549: Add support for prefix-only CLI options.

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.

Jan 10 2019, 8:46 AM
jdenny added a comment to D56541: [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and `FILECHECK_OPTS` into environment for FileCheck tests..

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.

Jan 10 2019, 8:20 AM
jdenny added inline comments to D55940: Detect incorrect FileCheck variable CLI definition.
Jan 10 2019, 7:49 AM
jdenny added a comment to D56541: [FileCheck] Don't propagate `FILECHECK_DUMP_INPUT_ON_FAILURE` and `FILECHECK_OPTS` into environment for FileCheck tests..

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

Jan 10 2019, 7:02 AM

Jan 9 2019

jdenny updated the diff for D55825: [FileCheck] Suppress old -v/-vv diags if dumping input.
  • 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.
Jan 9 2019, 3:30 PM
jdenny added inline comments to D55825: [FileCheck] Suppress old -v/-vv diags if dumping input.
Jan 9 2019, 11:15 AM

Jan 8 2019

jdenny added a comment to D55825: [FileCheck] Suppress old -v/-vv diags if dumping input.

Thanks for reviewing!

Jan 8 2019, 3:35 PM
jdenny added a comment to D55825: [FileCheck] Suppress old -v/-vv diags if dumping input.

Ping.

Jan 8 2019, 9:38 AM

Jan 4 2019

jdenny committed rL350441: [OpenMP] Refactor const restriction for linear.
[OpenMP] Refactor const restriction for linear
Jan 4 2019, 2:16 PM
jdenny committed rC350441: [OpenMP] Refactor const restriction for linear.
[OpenMP] Refactor const restriction for linear
Jan 4 2019, 2:16 PM
jdenny committed rL350440: [OpenMP] Refactor const restriction for reductions.
[OpenMP] Refactor const restriction for reductions
Jan 4 2019, 2:16 PM
jdenny closed D56299: [OpenMP] Refactor const restriction for linear.
Jan 4 2019, 2:16 PM
jdenny committed rC350440: [OpenMP] Refactor const restriction for reductions.
[OpenMP] Refactor const restriction for reductions
Jan 4 2019, 2:16 PM
jdenny closed D56298: [OpenMP] Refactor const restriction for reductions.
Jan 4 2019, 2:15 PM
jdenny committed rL350439: [OpenMP] Replace predetermined shared for const variable.
[OpenMP] Replace predetermined shared for const variable
Jan 4 2019, 2:15 PM
jdenny committed rC350439: [OpenMP] Replace predetermined shared for const variable.
[OpenMP] Replace predetermined shared for const variable
Jan 4 2019, 2:15 PM
jdenny closed D56113: [OpenMP] Replace predetermined shared for const variable.
Jan 4 2019, 2:15 PM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

Thanks for the review!

Jan 4 2019, 12:46 PM
jdenny set the repository for D56298: [OpenMP] Refactor const restriction for reductions to rC Clang.
Jan 4 2019, 12:27 PM
jdenny updated the diff for D56299: [OpenMP] Refactor const restriction for linear.

Update for changes to earlier patches in series.

Jan 4 2019, 12:27 PM
jdenny set the repository for D56299: [OpenMP] Refactor const restriction for linear to rC Clang.
Jan 4 2019, 12:27 PM
jdenny updated the diff for D56298: [OpenMP] Refactor const restriction for reductions.

Update for changes to earlier patch in series.

Jan 4 2019, 12:27 PM
jdenny updated the diff for D56113: [OpenMP] Replace predetermined shared for const variable.

Split checkConstNotMutableType implementation.

Jan 4 2019, 12:27 PM
jdenny updated the diff for D56299: [OpenMP] Refactor const restriction for linear.

Update for changes to earlier patches in series.

Jan 4 2019, 11:08 AM
jdenny updated the diff for D56298: [OpenMP] Refactor const restriction for reductions.

Update for changes to earlier patch in series.

Jan 4 2019, 11:04 AM
jdenny updated the diff for D56113: [OpenMP] Replace predetermined shared for const variable.
  • 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.
Jan 4 2019, 11:03 AM
jdenny added inline comments to D56113: [OpenMP] Replace predetermined shared for const variable.
Jan 4 2019, 7:49 AM

Jan 3 2019

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

Rebased. Ping.

Jan 3 2019, 7:58 PM · Restricted Project
jdenny added inline comments to D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.
Jan 3 2019, 6:23 PM
jdenny committed rOMP350377: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.
[OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu
Jan 3 2019, 6:11 PM
jdenny committed rL350377: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.
[OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu
Jan 3 2019, 6:11 PM
jdenny closed D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.
Jan 3 2019, 6:11 PM
jdenny added inline comments to D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.
Jan 3 2019, 4:06 PM
jdenny added a child revision for D56298: [OpenMP] Refactor const restriction for reductions: D56299: [OpenMP] Refactor const restriction for linear.
Jan 3 2019, 3:38 PM
jdenny added a parent revision for D56299: [OpenMP] Refactor const restriction for linear: D56298: [OpenMP] Refactor const restriction for reductions.
Jan 3 2019, 3:38 PM
jdenny created D56299: [OpenMP] Refactor const restriction for linear.
Jan 3 2019, 3:37 PM
jdenny added a parent revision for D56298: [OpenMP] Refactor const restriction for reductions: D56113: [OpenMP] Replace predetermined shared for const variable.
Jan 3 2019, 3:36 PM
jdenny added a child revision for D56113: [OpenMP] Replace predetermined shared for const variable: D56298: [OpenMP] Refactor const restriction for reductions.
Jan 3 2019, 3:36 PM
jdenny created D56298: [OpenMP] Refactor const restriction for reductions.
Jan 3 2019, 3:36 PM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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.

Jan 3 2019, 2:24 PM
jdenny updated the summary of D56113: [OpenMP] Replace predetermined shared for const variable.
Jan 3 2019, 2:21 PM
jdenny updated the diff for D56113: [OpenMP] Replace predetermined shared for const variable.
  • 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.
Jan 3 2019, 2:20 PM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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.

Jan 3 2019, 12:52 PM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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.

Jan 3 2019, 12:34 PM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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.

Jan 3 2019, 12:11 PM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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.

Jan 3 2019, 10:34 AM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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.

Jan 3 2019, 10:19 AM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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.

Jan 3 2019, 8:36 AM

Jan 2 2019

jdenny added inline comments to D55940: Detect incorrect FileCheck variable CLI definition.
Jan 2 2019, 2:42 PM
jdenny added a comment to D55940: Detect incorrect FileCheck variable CLI definition.

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.

Jan 2 2019, 2:38 PM
jdenny added a comment to D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.

Ping?

Jan 2 2019, 2:09 PM
jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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

Jan 2 2019, 1:50 PM
jdenny updated the diff for D56113: [OpenMP] Replace predetermined shared for const variable.
  • Apply reviewer suggestions.
  • Add test case for change in default(none) behavior.
Jan 2 2019, 1:49 PM

Dec 31 2018

jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

Hi Alexey,

Dec 31 2018, 9:47 AM

Dec 28 2018

jdenny added a comment to D56113: [OpenMP] Replace predetermined shared for const variable.

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

Dec 28 2018, 2:32 PM

Dec 27 2018

jdenny created D56113: [OpenMP] Replace predetermined shared for const variable.
Dec 27 2018, 2:59 PM

Dec 21 2018

jdenny added a comment to D55940: Detect incorrect FileCheck variable CLI definition.

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

Dec 21 2018, 2:05 PM

Dec 20 2018

jdenny added a comment to D55940: Detect incorrect FileCheck variable CLI definition.

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

Dec 20 2018, 12:18 PM
jdenny added a comment to D55940: Detect incorrect FileCheck variable CLI definition.

Better diagnostics would definitely be helpful here.

Dec 20 2018, 12:13 PM
jdenny updated the diff for D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.

Apply variable renames suggested by grokos.

Dec 20 2018, 7:56 AM

Dec 19 2018

jdenny added inline comments to D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.
Dec 19 2018, 3:27 PM
jdenny committed rL349635: [OpenMP] Fix data sharing analysis in nested clause.
[OpenMP] Fix data sharing analysis in nested clause
Dec 19 2018, 8:03 AM
jdenny committed rC349635: [OpenMP] Fix data sharing analysis in nested clause.
[OpenMP] Fix data sharing analysis in nested clause
Dec 19 2018, 8:03 AM
jdenny closed D55861: [OpenMP] Fix data sharing analysis in nested clause.
Dec 19 2018, 8:03 AM

Dec 18 2018

jdenny created D55861: [OpenMP] Fix data sharing analysis in nested clause.
Dec 18 2018, 3:42 PM
jdenny updated the diff for D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu.
  • Fix a comment typo.
Dec 18 2018, 8:07 AM
jdenny created D55825: [FileCheck] Suppress old -v/-vv diags if dumping input.
Dec 18 2018, 7:05 AM

Dec 17 2018

jdenny committed rL349432: [FileCheck] Try to fix test on windows due to r349418.
[FileCheck] Try to fix test on windows due to r349418
Dec 17 2018, 5:20 PM
jdenny committed rL349425: [FileCheck] Annotate input dump (final tweaks).
[FileCheck] Annotate input dump (final tweaks)
Dec 17 2018, 4:07 PM
jdenny closed D55738: [FileCheck] Annotate input dump (final tweaks).
Dec 17 2018, 4:07 PM