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 (62 w, 5 d)

Recent Activity

Mon, Jan 14

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

Thu, Jan 10

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

Thu, Jan 10, 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.

Thu, Jan 10, 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.

Thu, Jan 10, 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.

Thu, Jan 10, 8:20 AM
jdenny added inline comments to D55940: Detect incorrect FileCheck variable CLI definition.
Thu, Jan 10, 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:

Thu, Jan 10, 7:02 AM

Wed, Jan 9

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

Tue, Jan 8

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

Thanks for reviewing!

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

Ping.

Tue, Jan 8, 9:38 AM

Fri, Jan 4

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

Thanks for the review!

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

Update for changes to earlier patches in series.

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

Update for changes to earlier patch in series.

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

Split checkConstNotMutableType implementation.

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

Update for changes to earlier patches in series.

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

Update for changes to earlier patch in series.

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

Thu, Jan 3

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

Rebased. Ping.

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

Thu, Jan 3, 2:24 PM
jdenny updated the summary of D56113: [OpenMP] Replace predetermined shared for const variable.
Thu, Jan 3, 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.
Thu, Jan 3, 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.

Thu, Jan 3, 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.

Thu, Jan 3, 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.

Thu, Jan 3, 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.

Thu, Jan 3, 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.

Thu, Jan 3, 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.

Thu, Jan 3, 8:36 AM

Wed, Jan 2

jdenny added inline comments to D55940: Detect incorrect FileCheck variable CLI definition.
Wed, Jan 2, 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.

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

Ping?

Wed, Jan 2, 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.

Wed, Jan 2, 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.
Wed, Jan 2, 1:49 PM

Mon, Dec 31

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

Hi Alexey,

Mon, Dec 31, 9:47 AM

Fri, Dec 28

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.

Fri, Dec 28, 2:32 PM

Thu, Dec 27

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

Fri, Dec 21

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

Fri, Dec 21, 2:05 PM

Thu, Dec 20

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

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

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

Better diagnostics would definitely be helpful here.

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

Apply variable renames suggested by grokos.

Thu, Dec 20, 7:56 AM

Wed, Dec 19

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

Tue, Dec 18

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

Mon, Dec 17

jdenny committed rL349432: [FileCheck] Try to fix test on windows due to r349418.
[FileCheck] Try to fix test on windows due to r349418
Mon, Dec 17, 5:20 PM
jdenny committed rL349425: [FileCheck] Annotate input dump (final tweaks).
[FileCheck] Annotate input dump (final tweaks)
Mon, Dec 17, 4:07 PM
jdenny closed D55738: [FileCheck] Annotate input dump (final tweaks).
Mon, Dec 17, 4:07 PM
jdenny committed rL349424: [FileCheck] Annotate input dump (7/7).
[FileCheck] Annotate input dump (7/7)
Mon, Dec 17, 4:06 PM
jdenny closed D53899: [FileCheck] Annotate input dump (7/7).
Mon, Dec 17, 4:06 PM
jdenny committed rL349423: [FileCheck] Annotate input dump (6/7).
[FileCheck] Annotate input dump (6/7)
Mon, Dec 17, 4:06 PM
jdenny closed D53898: [FileCheck] Annotate input dump (6/7).
Mon, Dec 17, 4:06 PM
jdenny committed rL349422: [FileCheck] Annotate input dump (5/7).
[FileCheck] Annotate input dump (5/7)
Mon, Dec 17, 4:06 PM
jdenny closed D53897: [FileCheck] Annotate input dump (5/7).
Mon, Dec 17, 4:06 PM
jdenny committed rL349421: [FileCheck] Annotate input dump (4/7).
[FileCheck] Annotate input dump (4/7)
Mon, Dec 17, 4:06 PM
jdenny closed D53896: [FileCheck] Annotate input dump (4/7).
Mon, Dec 17, 4:06 PM
jdenny committed rL349420: [FileCheck] Annotate input dump (3/7).
[FileCheck] Annotate input dump (3/7)
Mon, Dec 17, 4:05 PM
jdenny closed D53894: [FileCheck] Annotate input dump (3/7).
Mon, Dec 17, 4:05 PM
jdenny committed rL349419: [FileCheck] Annotate input dump (2/7).
[FileCheck] Annotate input dump (2/7)
Mon, Dec 17, 4:05 PM
jdenny closed D53893: [FileCheck] Annotate input dump (2/7).
Mon, Dec 17, 4:05 PM
jdenny committed rL349418: [FileCheck] Annotate input dump (1/7).
[FileCheck] Annotate input dump (1/7)
Mon, Dec 17, 4:05 PM
jdenny closed D52999: [FileCheck] Annotate input dump (1/7).
Mon, Dec 17, 4:05 PM

Dec 15 2018

jdenny added inline comments to D53898: [FileCheck] Annotate input dump (6/7).
Dec 15 2018, 5:57 AM
jdenny added a comment to D53897: [FileCheck] Annotate input dump (5/7).

Remaining suggestions done in D55738.

Dec 15 2018, 5:57 AM
jdenny added inline comments to D52999: [FileCheck] Annotate input dump (1/7).
Dec 15 2018, 5:54 AM
jdenny added a child revision for D53899: [FileCheck] Annotate input dump (7/7): D55738: [FileCheck] Annotate input dump (final tweaks).
Dec 15 2018, 5:54 AM
jdenny added a parent revision for D55738: [FileCheck] Annotate input dump (final tweaks): D53899: [FileCheck] Annotate input dump (7/7).
Dec 15 2018, 5:54 AM
jdenny created D55738: [FileCheck] Annotate input dump (final tweaks).
Dec 15 2018, 5:52 AM

Dec 14 2018

jdenny added a reviewer for D55588: [OpenMP] Fix nvidia-cuda-toolkit detection on Debian/Ubuntu: AndreyChurbanov.
Dec 14 2018, 3:00 PM
jdenny added a reviewer for D55725: [OpenMP] Add libs to clang-dedicated directories: AndreyChurbanov.
Dec 14 2018, 2:59 PM
jdenny created D55725: [OpenMP] Add libs to clang-dedicated directories.
Dec 14 2018, 2:58 PM
jdenny added inline comments to D53899: [FileCheck] Annotate input dump (7/7).
Dec 14 2018, 1:31 PM
jdenny added a comment to D53897: [FileCheck] Annotate input dump (5/7).

The terminology around "possible" "expected" and "final" matches probably warrants a few lines of commentary somewhere, unless I missed it in an earlier patch. There are getting to be enough cases that the casual reader (ahem) can't just Zen their way through them all.

Dec 14 2018, 1:18 PM
jdenny added inline comments to D53898: [FileCheck] Annotate input dump (6/7).
Dec 14 2018, 1:12 PM

Dec 12 2018

jdenny added inline comments to D52999: [FileCheck] Annotate input dump (1/7).
Dec 12 2018, 3:18 PM