Reformatted documentation change to fit better on the page.

In D104566#2831315, @MaskRay wrote:

LG

Applied jhuber6's suggestions:

In D104559#2827772, @jhuber6 wrote:

Thanks for addressing this. Please update the documentation as well ./openmp/docs/design/Runtimes.rst.

LGTM

Thanks for all that work on the other tests! Still LGTM.

Ah, I see. Even though it wouldn't add very much code, it sounds like we agree that the conceptual complexity isn't worthwhile given the direction we'd like to go eventually. Thanks again for looking into it.

In D98859#2662417, @mstorsjo wrote:

Improved the testcases for '!'.

In D86222#2654396, @thopre wrote:

It seems the main issue left to address is tests that this patch breaks.

In D98859#2652464, @mstorsjo wrote:

@jdenny - Updated with some tests. The existing tests in the shtest-not suite seem to cover the uses of 'not' quite well - so just for the change of making 'not' evaluated builtin those existing tests should cover all aspects I think.

You say there are many test failures. Should I hold off reviewing the test changes already in this patch, or are those fine?

I haven't looked through the tests yet, but the implementation logic looks right to me. Thanks!

Rebased, and addressed reviewer comments.

Please add tests before committing this again.

In D98691#2634161, @thopre wrote:

Sorry it seems I forgot to run the whole testsuite when I put this up for review. The patch found 3 issues:

https://reviews.llvm.org/D98852
https://reviews.llvm.org/D98853
https://reviews.llvm.org/D98854

Ping.

Thanks for the review. I'm rebasing and retesting, and then I'll push.

LGTM if @jhenderson is happy.

Address @jhenderson's review.

In D98343#2618993, @thopre wrote:

Maybe I should drop this change and just find a better name for checkWildcardRegexCharMatchFailure.

In D98086#2618793, @jhenderson wrote:

Yes, sorry, been busy. I was off sick much of last week, and am participating in the ACCU conference most of this week. I'll try to find time to look at this next week, if you want my input, but there's no need to wait on me otherwise.

LGTM. Thanks!

Based on the comments in D97845, I see the semantic difference this patch addresses, but....

Other than a comment request, LGTM.

Rebased for contextual changes from D98086.

Applied @thopre's suggestions.

In D98086#2611178, @thopre wrote:

I like the idea behind it. I suppose that's what you had in mind when you wrote your comments in https://reviews.llvm.org/D86222. It should make that patch a lot simpler.

I have no immediate plans to pursue this again, but maybe one day. Feel free to take over. Be sure to read the RFC thread. There was some delay waiting on another patch from someone else, but I've lost track.

Rebased for changes to D98086.

I've generalized this patch to also handle errors that occur after matching. One issue is that PrintNoMatch was handling those, but I moved the handling to PrintMatch so that we actually report the full match that was found. That seems like useful information and helps with -dump-input.

In D97046#2609763, @davezarzycki wrote:

I have a patch that implements the record-and-reorder feature. It turned out to be simpler than I feared. Now I just need to figure out how to write a test for it that makes sense and is reliable. For ninja check-llvm, the record-and-reorder mode completes the test suite 51% faster on my 48-core machine. I think it's time to abandon this change proposal.

Actually, I do need to rethink the logic for that overflow case: PrintNoMatch is being called after a match, so its diagnostics don't make sense in that case. Sorry for the noise. I'll work on it.

This patch also extends input dumps to include similar error
diagnostics.

As discussed, I'll try a different solution for this issue this patch addresses, probably in a different review.

Rebased and updated for D93341's effect on -dump-input.

Thanks for the quick review. Sorry for the delay in pushing.

Thanks for the reviews!

In D97616#2600586, @grokos wrote:

In D97616#2597237, @jdenny wrote:

• The modified functions all have a common prologue. In a separate patch, I think that most of that should be moved to CheckDeviceAndCtors. Agreed?

Sure!

• Rebased.
• Added test for mandatory offload with omp_get_num_devices() == 0.
• Added comments pointing to OpenMP spec issue 2669.

In D97472#2600955, @abhina.sreeskantharajan wrote:

In D97472#2600526, @jdenny wrote:

I may be lost. Why is --ignore-case still needed in llvm/test/tools/yaml2obj/ELF/DWARF/debug-gnu-pubnames.yaml? I thought the special case for win32 was supposed to handle that.

I missed this testcase in my old patch. I noticed it because it started to fail when I tried to add --ignore-case as a solution. I can fix it in this patch as well if desired or open a separate patch for it after this one.

I may be lost. Why is --ignore-case still needed in llvm/test/tools/yaml2obj/ELF/DWARF/debug-gnu-pubnames.yaml? I thought the special case for win32 was supposed to handle that.

@thopre It's been almost a month. I suggest we move forward with this and address any complaints post-commit.

In D96653#2597583, @thopre wrote:

What does the label:2 X error: no match found mean then? I thought this was (despite the prefix) applying to the CHECK since the CHECK is searched after the LABEL.

In D96653#2597491, @probinson wrote:

As far as I know, it does. However, before CHECK-LABEL, a CHECK-NOT range is not the same as a CHECK range:

So if you have

CHECK-NOT: bar
CHECK-LABEL: bar

it will invariably fail? (Trying to distinguish behavior of the directives from behavior of the dump output.)

One more question: How do I test the new check? For cuda offload, I can set CUDA_VISIBLE_DEVICES to an empty string, but I haven't found a general way.

In anticipation of the spec change, I've updated the patch to check the omp_get_num_devices() == 0 case. A few questions:

Thanks for pointing that out (OpenMP spec issue 2669). I wasn't aware it was changing. I'll wait until that settles.

In D96653#2594600, @probinson wrote:

The strange part to me is that a preceding directive's search range ends at the end of the CHECK-LABEL match. It seems like it ought to end at the start of the CHECK-LABEL match.

I'd expect the behavior of CHECK-NOT ranges to work the same for CHECK-LABEL as for a regular CHECK.

In D97616#2595403, @RaviNarayanaswamy wrote:

If OMP_TARGET_OFFLOAD=mandatory it will not fail but just return

Thanks!

In every case where this patch inserts a check, there is code immediately following it that calls HandleTargetOutcome(false, loc);, usually because CheckDeviceAndCtors(device_id) != OFFLOAD_SUCCESS. The inserted check prevents that from code from executing. Is that what you're asking?

While it's beyond what I need at the moment, I agree that test_phases looks better than early_tests, late_tests, etc.

If a lit invocation runs multiple test suites that contain identically named test files, is there any way to make an --xfail entry apply to only one of them?