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.

In D66574#1666005, @stella.stamenova wrote:

LGTM as long as all the tests still pass

In D66531#1665158, @rnk wrote:

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

Is anything blocking this?

In D66574#1643790, @jdenny wrote:

I made several changes:

• Added missing functools

I made several changes:

In D66574#1641050, @probinson wrote:

+Maggie who did the original diff implementation, AFAICT.

In D65697#1640202, @probinson wrote:

In D65697#1640136, @jdenny wrote:

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.

Thanks.

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

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.

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.

Make suggested changes for passing around the capture level.

Restore previous version of patch, and rebase.

In D65835#1639700, @ABataev wrote:

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.

In D65835#1639622, @ABataev wrote:

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.

In D65835#1639612, @ABataev wrote:

In D65835#1639593, @jdenny wrote:

In D65835#1639585, @ABataev wrote:

In D65835#1639584, @jdenny wrote:

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.

In D65835#1639585, @ABataev wrote:

In D65835#1639584, @jdenny wrote:

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.

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:

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

In D65697#1639463, @probinson wrote:

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.

Ping.

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

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

In D65697#1638518, @rnk wrote:

lgtm

Now this one is super simple. :)

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.

Rebase onto D66506.

Replace ValueError with InternalShellError.

In D65697#1638038, @probinson wrote:

In D65697#1638018, @probinson wrote:

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.

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.

Thanks!

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

• Rebased.
• Renamed %TestFileCheckOut to %ProtectFileCheckOutput.
• Added documentation to llvm/docs/TestingGuide.rst.

In D66293#1635995, @probinson wrote:

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

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

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.

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

In D66247#1630352, @ABataev wrote:

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.

In D66247#1630321, @ABataev wrote:

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.

In D66247#1630262, @ABataev wrote:

In D66247#1630245, @jdenny wrote:

In D66247#1630196, @ABataev wrote:

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.

In D66247#1630196, @ABataev wrote:

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.