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 (98 w, 3 d)

Recent Activity

Fri, Sep 20

jdenny added inline comments to D67643: [lit] Extend internal diff to support `-` argument.
Fri, Sep 20, 6:56 AM · Restricted Project

Mon, Sep 16

jdenny removed a child revision for D66574: [lit] Make internal diff work in pipelines: D66506: [lit] Fix internal env calling other internal commands.
Mon, Sep 16, 5:22 PM · Restricted Project
jdenny removed a parent revision for D66506: [lit] Fix internal env calling other internal commands: D66574: [lit] Make internal diff work in pipelines.
Mon, Sep 16, 5:22 PM · Restricted Project
jdenny added a parent revision for D67643: [lit] Extend internal diff to support `-` argument: D66574: [lit] Make internal diff work in pipelines.
Mon, Sep 16, 5:22 PM · Restricted Project
jdenny added a child revision for D67643: [lit] Extend internal diff to support `-` argument: D66506: [lit] Fix internal env calling other internal commands.
Mon, Sep 16, 5:22 PM · Restricted Project
jdenny added a child revision for D66574: [lit] Make internal diff work in pipelines: D67643: [lit] Extend internal diff to support `-` argument.
Mon, Sep 16, 5:22 PM · Restricted Project
jdenny added a parent revision for D66506: [lit] Fix internal env calling other internal commands: D67643: [lit] Extend internal diff to support `-` argument.
Mon, Sep 16, 5:22 PM · Restricted Project
jdenny created D67643: [lit] Extend internal diff to support `-` argument.
Mon, Sep 16, 5:13 PM · Restricted Project
jdenny added a comment to D66574: [lit] Make internal diff work in pipelines.

I had to revert this because it broke a Windows bot. I didn't consider that this patch not only enables internal diff to function in pipelines but also causes it to be used instead of external diffs when in pipelines. Thus, it cannot be pushed without additional patches that add features like -, which are sometimes used in the test suite when diff appears in pipelines.

Mon, Sep 16, 4:59 PM · Restricted Project
jdenny committed rG0a0ea7ec99d4: Revert r372035: "[lit] Make internal diff work in pipelines" (authored by jdenny).
Revert r372035: "[lit] Make internal diff work in pipelines"
Mon, Sep 16, 4:47 PM
jdenny committed rL372051: Revert r372035: "[lit] Make internal diff work in pipelines".
Revert r372035: "[lit] Make internal diff work in pipelines"
Mon, Sep 16, 4:46 PM
jdenny committed rG2152ae985c28: [lit] Make internal diff work in pipelines (authored by jdenny).
[lit] Make internal diff work in pipelines
Mon, Sep 16, 2:28 PM
jdenny committed rL372035: [lit] Make internal diff work in pipelines.
[lit] Make internal diff work in pipelines
Mon, Sep 16, 2:27 PM
jdenny closed D66574: [lit] Make internal diff work in pipelines.
Mon, Sep 16, 2:27 PM · Restricted Project

Wed, Sep 11

jdenny added a comment to D66574: [lit] Make internal diff work in pipelines.

LGTM as long as all the tests still pass

Wed, Sep 11, 7:38 AM · Restricted Project

Tue, Sep 10

jdenny added a comment to D66531: [lit] Fix `not` calling internal commands.
In D66531#1665158, @rnk wrote:

I managed to run into this again:
https://reviews.llvm.org/rL371478#687359

Is anything blocking this?

Tue, Sep 10, 2:33 PM · Restricted Project

Mon, Sep 2

jdenny committed rL370686: Request commit access for jdenny.
Request commit access for jdenny
Mon, Sep 2, 2:16 PM

Sun, Aug 25

jdenny updated the summary of D66574: [lit] Make internal diff work in pipelines.
Sun, Aug 25, 7:22 AM · Restricted Project

Aug 23 2019

jdenny added inline comments to D66574: [lit] Make internal diff work in pipelines.
Aug 23 2019, 4:16 PM · Restricted Project
jdenny added a comment to D66574: [lit] Make internal diff work in pipelines.

I made several changes:

  • Added missing functools
Aug 23 2019, 4:05 PM · Restricted Project
jdenny updated the diff for D66574: [lit] Make internal diff work in pipelines.

I made several changes:

Aug 23 2019, 4:05 PM · Restricted Project

Aug 22 2019

jdenny added a comment to D66574: [lit] Make internal diff work in pipelines.

+Maggie who did the original diff implementation, AFAICT.

Aug 22 2019, 7:35 AM · Restricted Project

Aug 21 2019

jdenny added a parent revision for D66531: [lit] Fix `not` calling internal commands: D65697: [lit] Fix internal env calling env.
Aug 21 2019, 8:49 PM · Restricted Project
jdenny added a child revision for D66574: [lit] Make internal diff work in pipelines: D66506: [lit] Fix internal env calling other internal commands.
Aug 21 2019, 8:49 PM · Restricted Project
jdenny added a child revision for D65697: [lit] Fix internal env calling env: D66531: [lit] Fix `not` calling internal commands.
Aug 21 2019, 8:49 PM · Restricted Project
jdenny added a parent revision for D66506: [lit] Fix internal env calling other internal commands: D66574: [lit] Make internal diff work in pipelines.
Aug 21 2019, 8:49 PM · Restricted Project
jdenny committed rG3c577bb415a5: [lit] Diagnose insufficient args to internal env (authored by jdenny).
[lit] Diagnose insufficient args to internal env
Aug 21 2019, 8:42 PM
jdenny committed rL369620: [lit] Diagnose insufficient args to internal env.
[lit] Diagnose insufficient args to internal env
Aug 21 2019, 8:41 PM
jdenny closed D66482: [lit] Diagnose insufficient args to internal env.
Aug 21 2019, 8:40 PM · Restricted Project
jdenny committed rG7d5bc554333e: [OpenMP] Permit map with DSA on combined directive (authored by jdenny).
[OpenMP] Permit map with DSA on combined directive
Aug 21 2019, 8:34 PM
jdenny committed rL369619: [OpenMP] Permit map with DSA on combined directive.
[OpenMP] Permit map with DSA on combined directive
Aug 21 2019, 8:33 PM
jdenny closed D65835: [OpenMP] Permit map with DSA on combined directive.
Aug 21 2019, 8:33 PM · Restricted Project, Restricted Project
jdenny created D66574: [lit] Make internal diff work in pipelines.
Aug 21 2019, 7:43 PM · Restricted Project
jdenny added a comment to D65697: [lit] Fix internal env calling env.
In D65697#1640109, @rnk wrote:

Most LLVM test suites are configured to use the internal shell on Windows. I think what's happening is that users happen to have an external copy of diff that gets called when it appears in a pipeline, or they are prefixed by not. The external diff is called, and things "work out" and we never noticed.

Are these patches going to break those tests on Windows? We now see diff anywhere in a pipeline, and we reject its appearance in a pipeline.

Aug 21 2019, 3:54 PM · Restricted Project
jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

Thanks.

Aug 21 2019, 3:44 PM · Restricted Project, Restricted Project
jdenny added inline comments to D65835: [OpenMP] Permit map with DSA on combined directive.
Aug 21 2019, 3:28 PM · Restricted Project, Restricted Project
jdenny updated the diff for D65835: [OpenMP] Permit map with DSA on combined directive.

Make suggested changes to default arguments, comments on literals, and parameter names.

Aug 21 2019, 3:25 PM · Restricted Project, Restricted Project
jdenny added a comment to D65697: [lit] Fix internal env calling env.
In D65697#1640109, @rnk wrote:

Most LLVM test suites are configured to use the internal shell on Windows. I think what's happening is that users happen to have an external copy of diff that gets called when it appears in a pipeline, or they are prefixed by not. The external diff is called, and things "work out" and we never noticed.

Aug 21 2019, 2:44 PM · Restricted Project
jdenny added a comment to D65697: [lit] Fix internal env calling env.
In D65697#1639974, @rnk wrote:

Based on what Paul said, it seems that we have some clang/LLVM tests that use internal diff and some that use real external diff in a pipeline, and we didn't know it.

Aug 21 2019, 2:04 PM · Restricted Project
jdenny updated the diff for D65835: [OpenMP] Permit map with DSA on combined directive.

Make suggested changes for passing around the capture level.

Aug 21 2019, 1:53 PM · Restricted Project, Restricted Project
jdenny added inline comments to D65835: [OpenMP] Permit map with DSA on combined directive.
Aug 21 2019, 1:21 PM · Restricted Project, Restricted Project
jdenny added inline comments to D65835: [OpenMP] Permit map with DSA on combined directive.
Aug 21 2019, 1:09 PM · Restricted Project, Restricted Project
jdenny updated the diff for D65835: [OpenMP] Permit map with DSA on combined directive.

Restore previous version of patch, and rebase.

Aug 21 2019, 11:58 AM · Restricted Project, Restricted Project
jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

Chwck 2 ifs: if (IsVariableUsedInMapClause) and the second if (IsByRef && Ty.getNonReferenceType()->isScalarType()). If the variable is mapped, IsByRef is set to true, but later it may be changed in the second if, when we check for the use in the firstprivate clause. We need to add a check, that the IsByRef must be set to false onlly if IsVariableUsedInMapClause is set to false too for combined target constructs.

Aug 21 2019, 11:22 AM · Restricted Project, Restricted Project
jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

We don't need this new level counter to correctly handle this situation. Just check for the combined target directive in Sema::isOpenMPCapturedByRef and return true if it is mapped as to from or just from. The change must be very simple.

Aug 21 2019, 11:05 AM · Restricted Project, Restricted Project
jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

I want to be sure we're on the same page. Due to the changes I just backed out, the following two examples now generate different code:

int a = 0;
#pragma omp target map(a)
#pragma omp teams firstprivate(a)
  ;
int a = 0;
#pragma omp target teams firstprivate(a) map(a)
  ;

The difference is whether a is passed by reference (the first case) or value (the second case) to the offloading function.

Is that fine for you?

No, this is what I warned about. We shall have the same codegen just like in the first case, the value must be passed by reference and mapped as tofrom.

If I add back those changes I just backed out, we get the same codegen. Is that what you want?

Those 2 cases must result in the same codegen. But I rather doubt we need your previous changes. Check Sema::isOpenMPCapturedByRef instead, required functionality must be handled in this function.

Aug 21 2019, 10:06 AM · Restricted Project, Restricted Project
jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

I want to be sure we're on the same page. Due to the changes I just backed out, the following two examples now generate different code:

int a = 0;
#pragma omp target map(a)
#pragma omp teams firstprivate(a)
  ;
int a = 0;
#pragma omp target teams firstprivate(a) map(a)
  ;

The difference is whether a is passed by reference (the first case) or value (the second case) to the offloading function.

Is that fine for you?

No, this is what I warned about. We shall have the same codegen just like in the first case, the value must be passed by reference and mapped as tofrom.

Aug 21 2019, 9:59 AM · Restricted Project, Restricted Project
jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

I want to be sure we're on the same page. Due to the changes I just backed out, the following two examples now generate different code:

Aug 21 2019, 9:54 AM · Restricted Project, Restricted Project
jdenny updated the diff for D65835: [OpenMP] Permit map with DSA on combined directive.

As requested, backed out the changes related to firstprivate scalars, and updated tests.

Aug 21 2019, 9:21 AM · Restricted Project, Restricted Project
jdenny added a comment to D65697: [lit] Fix internal env calling env.

Do people know why an internal diff was implemented originally? Is there a portability or performance issue?

See D39567 and in particular an enthused zturner looked forward to the day when all "shell" commands are internal, eliminating cross-platform issues and most if not all dependencies on additional packages other than Python (e.g. GnuWin32 on Windows). The only external commands would be LLVM-built tools/utilities. I see rnk participated in that review (late 2017).

If it's worthwhile to have an internal diff, is it worthwhile for it to support everything external diffs do? All pipes and redirects could be replaced by temporary files.

Creating temp files is slower than piping, given we have tens of thousands of tests it feels worthwhile to support piping. For diff in particular I found only one (I think unnecessary) redirect so not supporting that seems reasonable.

Aug 21 2019, 9:21 AM · Restricted Project
jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

Ping.

Aug 21 2019, 6:55 AM · Restricted Project, Restricted Project
jdenny created D66531: [lit] Fix `not` calling internal commands.
Aug 21 2019, 6:55 AM · Restricted Project
jdenny updated the diff for D65697: [lit] Fix internal env calling env.

As discussed, moved relevant tests and parts of summary to D66506.

Aug 21 2019, 6:46 AM · Restricted Project
jdenny updated the diff for D66506: [lit] Fix internal env calling other internal commands.

As discussed, moved relevant tests from D65697 to this patch, and rewrote summary to describe the non-NFC parts of this change.

Aug 21 2019, 6:37 AM · Restricted Project

Aug 20 2019

jdenny added a comment to D65697: [lit] Fix internal env calling env.
In D65697#1638518, @rnk wrote:

lgtm

Now this one is super simple. :)

Aug 20 2019, 5:00 PM · Restricted Project
jdenny added a comment to D66506: [lit] Fix internal env calling other internal commands.
In D66506#1638493, @rnk wrote:

lgtm

I suppose you could add a test for composing env with in process builtins, but I think that's already in the next patch.

Aug 20 2019, 4:56 PM · Restricted Project
jdenny added inline comments to D65697: [lit] Fix internal env calling env.
Aug 20 2019, 4:38 PM · Restricted Project
jdenny added a parent revision for D65697: [lit] Fix internal env calling env: D66506: [lit] Fix internal env calling other internal commands.
Aug 20 2019, 4:32 PM · Restricted Project
jdenny added a child revision for D66506: [lit] Fix internal env calling other internal commands: D65697: [lit] Fix internal env calling env.
Aug 20 2019, 4:32 PM · Restricted Project
jdenny created D66506: [lit] Fix internal env calling other internal commands.
Aug 20 2019, 4:32 PM · Restricted Project
jdenny updated the diff for D65697: [lit] Fix internal env calling env.

Rebase onto D66506.

Aug 20 2019, 4:32 PM · Restricted Project
jdenny added inline comments to D65697: [lit] Fix internal env calling env.
Aug 20 2019, 2:21 PM · Restricted Project
jdenny updated the diff for D65697: [lit] Fix internal env calling env.

Replace ValueError with InternalShellError.

Aug 20 2019, 2:21 PM · Restricted Project
jdenny added a comment to D65697: [lit] Fix internal env calling env.

I found one example of redirecting diff's output (to /dev/null).

This one is in llvm/test/tools/llvm-cov/llvm-cov.test (line 91): blah | not diff -u somefile - >/dev/null which IMO could use -q and not redirect.

Aug 20 2019, 1:55 PM · Restricted Project
jdenny updated the diff for D66482: [lit] Diagnose insufficient args to internal env.

Replace ValueError with InternalShellError as just discussed in D65697. I'll wait a little while to push in case someone wants to comment on this.

Aug 20 2019, 12:29 PM · Restricted Project
jdenny added inline comments to D65697: [lit] Fix internal env calling env.
Aug 20 2019, 12:18 PM · Restricted Project
jdenny added inline comments to D65697: [lit] Fix internal env calling env.
Aug 20 2019, 11:44 AM · Restricted Project
jdenny added a comment to D66482: [lit] Diagnose insufficient args to internal env.

Thanks!

Aug 20 2019, 11:11 AM · Restricted Project
jdenny added a child revision for D66482: [lit] Diagnose insufficient args to internal env: D65697: [lit] Fix internal env calling env.
Aug 20 2019, 9:30 AM · Restricted Project
jdenny added a parent revision for D65697: [lit] Fix internal env calling env: D66482: [lit] Diagnose insufficient args to internal env.
Aug 20 2019, 9:30 AM · Restricted Project
jdenny updated the diff for D65697: [lit] Fix internal env calling env.

Rebased onto D66482, and added a new test here for nested env without arguments.

Aug 20 2019, 9:30 AM · Restricted Project
jdenny created D66482: [lit] Diagnose insufficient args to internal env.
Aug 20 2019, 9:24 AM · Restricted Project
jdenny added inline comments to D65697: [lit] Fix internal env calling env.
Aug 20 2019, 7:02 AM · Restricted Project
jdenny updated the diff for D65121: [FileCheck] Make FILECHECK_OPTS useful for its test suite.
  • Rebased.
  • Renamed %TestFileCheckOut to %ProtectFileCheckOutput.
  • Added documentation to llvm/docs/TestingGuide.rst.
Aug 20 2019, 5:59 AM · Restricted Project

Aug 19 2019

jdenny committed rG971a9f7eea31: [lit] Check for accidental external command calls (authored by jdenny).
[lit] Check for accidental external command calls
Aug 19 2019, 4:00 PM
jdenny committed rL369309: [lit] Check for accidental external command calls.
[lit] Check for accidental external command calls
Aug 19 2019, 3:59 PM
jdenny closed D66293: [lit] Check for accidental external command calls.
Aug 19 2019, 3:59 PM · Restricted Project
jdenny added a comment to D66293: [lit] Check for accidental external command calls.

IIUC this means if anyone tries to write a RUN line that pipes something to diff or cd or whatever, it would explicitly fail.

Aug 19 2019, 3:19 PM · Restricted Project

Aug 16 2019

jdenny added inline comments to D66141: [FileCheck] Forbid using var defined on same line.
Aug 16 2019, 6:52 AM · Restricted Project

Aug 15 2019

jdenny committed rG9be6d7edb20b: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug (authored by jdenny).
[Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug
Aug 15 2019, 2:18 PM
jdenny committed rL369049: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug.
[Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug
Aug 15 2019, 2:18 PM
jdenny closed D61466: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug.
Aug 15 2019, 2:18 PM · Restricted Project, Restricted Project
jdenny added inline comments to D66141: [FileCheck] Forbid using var defined on same line.
Aug 15 2019, 10:26 AM · Restricted Project
jdenny abandoned D66247: [OpenMP] Fix target map for unused variables.
Aug 15 2019, 7:31 AM · Restricted Project
jdenny created D66293: [lit] Check for accidental external command calls.
Aug 15 2019, 7:26 AM · Restricted Project
jdenny updated the summary of D65697: [lit] Fix internal env calling env.
Aug 15 2019, 7:11 AM · Restricted Project
jdenny updated the diff for D65697: [lit] Fix internal env calling env.

Rebase. Remove checking for accidental external command calls. I'll put that in a separate patch.

Aug 15 2019, 7:10 AM · Restricted Project

Aug 14 2019

jdenny added a comment to D65835: [OpenMP] Permit map with DSA on combined directive.

I think you're referring to stuff like OpenMPCaptureLevel in ScopeInfo.h. I wrote those changes for this patch first, as can be seen in phabricator history. I needed them for the other patch too, and I thought we were going to end up applying that patch first, so I was going to strip them out here. Of course, as I understand it, the plan now is to apply only this patch.

Aug 14 2019, 6:12 PM · Restricted Project, Restricted Project
jdenny updated the diff for D65835: [OpenMP] Permit map with DSA on combined directive.

Rebase. Add more tests, as requested at D66247#1630441.

Aug 14 2019, 5:58 PM · Restricted Project, Restricted Project
jdenny added a comment to D66247: [OpenMP] Fix target map for unused variables.

Yes, just realized that, defaultmap does not affect explicit firstprivates. Then just check map(a) firstprivate(a) for int128 type. Check that it still has tofrom mapping. If so, then probably we already can handle such combinations correctly in the codegen. Also, test it for other mapping types, like alloc, from, to.

Aug 14 2019, 2:58 PM · Restricted Project
jdenny added a comment to D66247: [OpenMP] Fix target map for unused variables.

Try map(a) firstprivate(a) defaultmap(scalar:tofrom), where a is int, for example. The variable must be mapped as tofrom in this case but, most probably, will be mapped as to.

Aug 14 2019, 2:34 PM · Restricted Project
jdenny added a comment to D66247: [OpenMP] Fix target map for unused variables.

Do we really need to map such variables? According to standard, "The map clause specifies how an original list item is mapped from the current task’s data environment to a corresponding list item in the device data environment of the device identified by the construct.", i.e. it does not map the variable, but "specifies" how to map it.

What you're asking for now contradicts what you asked for at https://reviews.llvm.org/D65835#1624659 when I asked about the same example of target map(a) enclosing teams private(a).

Seems to me I did not expressed my idea absolutely correctly. I meant that if the variable is really mapped, then the maptype must be generated in all cases. If we do not need to map the variable, just like in these case, there definitely should not be maptype.

Aug 14 2019, 2:15 PM · Restricted Project
jdenny added a comment to D66247: [OpenMP] Fix target map for unused variables.

Do we really need to map such variables? According to standard, "The map clause specifies how an original list item is mapped from the current task’s data environment to a corresponding list item in the device data environment of the device identified by the construct.", i.e. it does not map the variable, but "specifies" how to map it.

Aug 14 2019, 1:58 PM · Restricted Project
jdenny created D66247: [OpenMP] Fix target map for unused variables.
Aug 14 2019, 1:09 PM · Restricted Project

Aug 13 2019

jdenny committed rGdbb757f46205: [FileCheck] Document FILECHECK_OPTS in -help (authored by jdenny).
[FileCheck] Document FILECHECK_OPTS in -help
Aug 13 2019, 8:00 PM
jdenny committed rG608f2bfd65ec: [FileCheck] Move -dump-input diagnostic to first line (authored by jdenny).
[FileCheck] Move -dump-input diagnostic to first line
Aug 13 2019, 8:00 PM
jdenny committed rL368787: [FileCheck] Document FILECHECK_OPTS in -help.
[FileCheck] Document FILECHECK_OPTS in -help
Aug 13 2019, 7:55 PM
jdenny closed D65707: [FileCheck] Document FILECHECK_OPTS in -help.
Aug 13 2019, 7:55 PM · Restricted Project
jdenny committed rL368786: [FileCheck] Move -dump-input diagnostic to first line.
[FileCheck] Move -dump-input diagnostic to first line
Aug 13 2019, 7:55 PM
jdenny closed D65702: [FileCheck] Move -dump-input diagnostic to first line.
Aug 13 2019, 7:55 PM · Restricted Project