[llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
Hi,

Using FileCheck, I have not found a way to make a group of CHECK-DAG directives match multiple occurrences of a string.  For example, I naively thought the following would match successfully:

```
$ cat checks.txt
// CHECK: start
// CHECK-DAG: foo
// CHECK-DAG: foo
// CHECK-DAG: bar
// CHECK-NEXT: end

$ cat input.txt
start
foo
bar
foo
end

$ FileCheck --input-file=input.txt checks.txt
checks.txt:5:16: error: CHECK-NEXT: is not on the line after the previous match
// CHECK-NEXT: end
               ^
input.txt:5:1: note: 'next' match was here
end
^
input.txt:3:4: note: previous match ended here
bar
   ^
input.txt:4:1: note: non-matching line after previous match is here
foo
^
```

The trouble is that both "CHECK-DAG: foo" directives match the first "foo".

I'd like this ability for testing a parallel program that outputs a series of non-unique strings in non-deterministic order.  Am I trying to push FileCheck beyond its intended domain?  Is there some existing feature for this purpose that I've overlooked?  If not, I see two potential solutions:

1. In a CHECK-DAG group, don't let the matches for patterns overlap.

2. Add a new CHECK-DAG-N directive, where N is some integer, to express that a pattern must have N non-overlapping matches.

An advantage of #1 that the intuitive way (at least in my mind) of expressing multiple occurrences of a string, as in the example above, would work.  An advantage of #2 is that existing CHECK-DAG functionality would not change, and so there should be no chance of impacting existing well formed tests.

To understand the issue better, I've prototyped #2.  It still needs test cases and documentation, so it's not ready for a formal patch review.  If people like the idea, I'll polish it up.

Thanks.

Joel

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
I would personally like a feature like that in FileCheck because it would make it a lot easier to write MachineOutliner tests, and would make the tests significantly smaller and easier to understand.

- Jessica

> On May 4, 2018, at 8:40 AM, Joel E. Denny via llvm-dev <[hidden email]> wrote:
>
> Hi,
>
> Using FileCheck, I have not found a way to make a group of CHECK-DAG directives match multiple occurrences of a string.  For example, I naively thought the following would match successfully:
>
> ```
> $ cat checks.txt
> // CHECK: start
> // CHECK-DAG: foo
> // CHECK-DAG: foo
> // CHECK-DAG: bar
> // CHECK-NEXT: end
>
> $ cat input.txt
> start
> foo
> bar
> foo
> end
>
> $ FileCheck --input-file=input.txt checks.txt
> checks.txt:5:16: error: CHECK-NEXT: is not on the line after the previous match
> // CHECK-NEXT: end
>                ^
> input.txt:5:1: note: 'next' match was here
> end
> ^
> input.txt:3:4: note: previous match ended here
> bar
>    ^
> input.txt:4:1: note: non-matching line after previous match is here
> foo
> ^
> ```
>
> The trouble is that both "CHECK-DAG: foo" directives match the first "foo".
>
> I'd like this ability for testing a parallel program that outputs a series of non-unique strings in non-deterministic order.  Am I trying to push FileCheck beyond its intended domain?  Is there some existing feature for this purpose that I've overlooked?  If not, I see two potential solutions:
>
> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
>
> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express that a pattern must have N non-overlapping matches.
>
> An advantage of #1 that the intuitive way (at least in my mind) of expressing multiple occurrences of a string, as in the example above, would work.  An advantage of #2 is that existing CHECK-DAG functionality would not change, and so there should be no chance of impacting existing well formed tests.
>
> To understand the issue better, I've prototyped #2.  It still needs test cases and documentation, so it's not ready for a formal patch review.  If people like the idea, I'll polish it up.
>
> Thanks.
>
> Joel
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
Hi Jessica,

This time I'm replying all....

On Fri, May 4, 2018 at 12:45 PM, Jessica Paquette <[hidden email]> wrote:
I would personally like a feature like that in FileCheck because it would make it a lot easier to write MachineOutliner tests, and would make the tests significantly smaller and easier to understand.

How do MachineOutliner tests accomplish this now?  Can you point me to an example?

Thanks.

Joel
 

- Jessica

> On May 4, 2018, at 8:40 AM, Joel E. Denny via llvm-dev <[hidden email]> wrote:
>
> Hi,
>
> Using FileCheck, I have not found a way to make a group of CHECK-DAG directives match multiple occurrences of a string.  For example, I naively thought the following would match successfully:
>
> ```
> $ cat checks.txt
> // CHECK: start
> // CHECK-DAG: foo
> // CHECK-DAG: foo
> // CHECK-DAG: bar
> // CHECK-NEXT: end
>
> $ cat input.txt
> start
> foo
> bar
> foo
> end
>
> $ FileCheck --input-file=input.txt checks.txt
> checks.txt:5:16: error: CHECK-NEXT: is not on the line after the previous match
> // CHECK-NEXT: end
>                ^
> input.txt:5:1: note: 'next' match was here
> end
> ^
> input.txt:3:4: note: previous match ended here
> bar
>    ^
> input.txt:4:1: note: non-matching line after previous match is here
> foo
> ^
> ```
>
> The trouble is that both "CHECK-DAG: foo" directives match the first "foo".
>
> I'd like this ability for testing a parallel program that outputs a series of non-unique strings in non-deterministic order.  Am I trying to push FileCheck beyond its intended domain?  Is there some existing feature for this purpose that I've overlooked?  If not, I see two potential solutions:
>
> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
>
> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express that a pattern must have N non-overlapping matches.
>
> An advantage of #1 that the intuitive way (at least in my mind) of expressing multiple occurrences of a string, as in the example above, would work.  An advantage of #2 is that existing CHECK-DAG functionality would not change, and so there should be no chance of impacting existing well formed tests.
>
> To understand the issue better, I've prototyped #2.  It still needs test cases and documentation, so it's not ready for a formal patch review.  If people like the idea, I'll polish it up.
>
> Thanks.
>
> Joel
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express
> that a pattern must have N non-overlapping matches.

I think #1 is much more intuitive and easy to describe/document than #2.
Changing the meaning of DAG in that way is highly unlikely to affect any
existing test, IMO.  And if it does, my first expectation is that the test
wasn't doing what the author intended anyway.
--paulr

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
Hi Paul,

On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express
> that a pattern must have N non-overlapping matches.

I think #1 is much more intuitive and easy to describe/document than #2.

Agreed.
 
Changing the meaning of DAG in that way is highly unlikely to affect any
existing test, IMO.  And if it does, my first expectation is that the test
wasn't doing what the author intended anyway.

I'm glad to hear that.  I'll look into #1.

Thanks.

Joel
 
--paulr



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
Right now, the tests try to accomplish the following

1. Define a sequence of instructions (e.g a,b,c)
2. Insert that sequence into k places with an unique instruction between them to make sure the outliner will yank them out *without overlaps*
3. Check for k calls to an outlined function
4. Check that the outlined sequence still exists in the program

This can result in some pretty long tests which would be a lot easier to define if we could say something like “this function must contain k instances of an outlined call, and not contain this sequence of instructions”.

test/CodeGen/machine-outliner.mir is a pretty good example of a test that would benefit from this sort of thing.

- Jessica

On May 4, 2018, at 10:05 AM, Joel E. Denny <[hidden email]> wrote:

Hi Jessica,

This time I'm replying all....

On Fri, May 4, 2018 at 12:45 PM, Jessica Paquette <[hidden email]> wrote:
I would personally like a feature like that in FileCheck because it would make it a lot easier to write MachineOutliner tests, and would make the tests significantly smaller and easier to understand.

How do MachineOutliner tests accomplish this now?  Can you point me to an example?

Thanks.

Joel
 

- Jessica

> On May 4, 2018, at 8:40 AM, Joel E. Denny via llvm-dev <[hidden email]> wrote:
>
> Hi,
>
> Using FileCheck, I have not found a way to make a group of CHECK-DAG directives match multiple occurrences of a string.  For example, I naively thought the following would match successfully:
>
> ```
> $ cat checks.txt
> // CHECK: start
> // CHECK-DAG: foo
> // CHECK-DAG: foo
> // CHECK-DAG: bar
> // CHECK-NEXT: end
>
> $ cat input.txt
> start
> foo
> bar
> foo
> end
>
> $ FileCheck --input-file=input.txt checks.txt
> checks.txt:5:16: error: CHECK-NEXT: is not on the line after the previous match
> // CHECK-NEXT: end
>                ^
> input.txt:5:1: note: 'next' match was here
> end
> ^
> input.txt:3:4: note: previous match ended here
> bar
>    ^
> input.txt:4:1: note: non-matching line after previous match is here
> foo
> ^
> ```
>
> The trouble is that both "CHECK-DAG: foo" directives match the first "foo".
>
> I'd like this ability for testing a parallel program that outputs a series of non-unique strings in non-deterministic order.  Am I trying to push FileCheck beyond its intended domain?  Is there some existing feature for this purpose that I've overlooked?  If not, I see two potential solutions:
>
> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
>
> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express that a pattern must have N non-overlapping matches.
>
> An advantage of #1 that the intuitive way (at least in my mind) of expressing multiple occurrences of a string, as in the example above, would work.  An advantage of #2 is that existing CHECK-DAG functionality would not change, and so there should be no chance of impacting existing well formed tests.
>
> To understand the issue better, I've prototyped #2.  It still needs test cases and documentation, so it's not ready for a formal patch review.  If people like the idea, I'll polish it up.
>
> Thanks.
>
> Joel
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev




_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
On Mon, May 7, 2018 at 1:23 PM, Jessica Paquette <[hidden email]> wrote:
Right now, the tests try to accomplish the following

1. Define a sequence of instructions (e.g a,b,c)
2. Insert that sequence into k places with an unique instruction between them to make sure the outliner will yank them out *without overlaps*
3. Check for k calls to an outlined function
4. Check that the outlined sequence still exists in the program

This can result in some pretty long tests which would be a lot easier to define if we could say something like “this function must contain k instances of an outlined call, and not contain this sequence of instructions”.

test/CodeGen/machine-outliner.mir is a pretty good example of a test that would benefit from this sort of thing.

I'm not sure if what I'm proposing will do what you need.  CHECK-DAG is for cases where a set of expected strings is unordered.  I'm trying to extend it to handle cases where those strings aren't unique either.  In your example, I see the non-unique property but not the unordered property.  Did I miss it?

Moreover, it seems your example could be handled now by two FileCheck calls with different prefixes, say OUT and INS.  The first would use a sequence of k "OUT:" directives to ensure that there are k outlined calls.  The second would use "INS-NOT:" directives to ensure other instructions don't occur.  The calls could also have a common prefix, say CHECK, to use for the function labels.  Does that do what you want?

Thanks

Joel

 

- Jessica

On May 4, 2018, at 10:05 AM, Joel E. Denny <[hidden email]> wrote:

Hi Jessica,

This time I'm replying all....

On Fri, May 4, 2018 at 12:45 PM, Jessica Paquette <[hidden email]> wrote:
I would personally like a feature like that in FileCheck because it would make it a lot easier to write MachineOutliner tests, and would make the tests significantly smaller and easier to understand.

How do MachineOutliner tests accomplish this now?  Can you point me to an example?

Thanks.

Joel
 

- Jessica

> On May 4, 2018, at 8:40 AM, Joel E. Denny via llvm-dev <[hidden email]> wrote:
>
> Hi,
>
> Using FileCheck, I have not found a way to make a group of CHECK-DAG directives match multiple occurrences of a string.  For example, I naively thought the following would match successfully:
>
> ```
> $ cat checks.txt
> // CHECK: start
> // CHECK-DAG: foo
> // CHECK-DAG: foo
> // CHECK-DAG: bar
> // CHECK-NEXT: end
>
> $ cat input.txt
> start
> foo
> bar
> foo
> end
>
> $ FileCheck --input-file=input.txt checks.txt
> checks.txt:5:16: error: CHECK-NEXT: is not on the line after the previous match
> // CHECK-NEXT: end
>                ^
> input.txt:5:1: note: 'next' match was here
> end
> ^
> input.txt:3:4: note: previous match ended here
> bar
>    ^
> input.txt:4:1: note: non-matching line after previous match is here
> foo
> ^
> ```
>
> The trouble is that both "CHECK-DAG: foo" directives match the first "foo".
>
> I'd like this ability for testing a parallel program that outputs a series of non-unique strings in non-deterministic order.  Am I trying to push FileCheck beyond its intended domain?  Is there some existing feature for this purpose that I've overlooked?  If not, I see two potential solutions:
>
> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
>
> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express that a pattern must have N non-overlapping matches.
>
> An advantage of #1 that the intuitive way (at least in my mind) of expressing multiple occurrences of a string, as in the example above, would work.  An advantage of #2 is that existing CHECK-DAG functionality would not change, and so there should be no chance of impacting existing well formed tests.
>
> To understand the issue better, I've prototyped #2.  It still needs test cases and documentation, so it's not ready for a formal patch review.  If people like the idea, I'll polish it up.
>
> Thanks.
>
> Joel
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev





_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express
> that a pattern must have N non-overlapping matches.

I think #1 is much more intuitive and easy to describe/document than #2.
Changing the meaning of DAG in that way is highly unlikely to affect any
existing test, IMO.  And if it does, my first expectation is that the test
wasn't doing what the author intended anyway.

I implemented #1, and I now have 105 new unexpected tests failures from llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1) test suites.

I've only looked at a few failures so far.  Of those, the problem is the expected one: in a CHECK-DAG group, multiple patterns match the same line, so not permitting overlapping matches causes some patterns never to match successfully.  However, all the overlapping matches so far seem unintentional.  That is, this new implementation seems to be catching test errors.

Here are some scenarios I found:

S1. Patterns sometimes identical
--------------------------------

For example, tools/clang/test/OpenMP/for_codegen.cpp has:

```
// TERM_DEBUG-DAG: [[DBG_LOC_START]] = !DILocation(line: [[@LINE-15]],
// TERM_DEBUG-DAG: [[DBG_LOC_END]] = !DILocation(line: [[@LINE-16]],
// TERM_DEBUG-DAG: [[DBG_LOC_CANCEL]] = !DILocation(line: [[@LINE-17]],
```

Patterns are the same except for the names of regex variables referenced.  The variables happen to have the same value, so the patterns match the same line when overlapping is permitted.

Perhaps the test was designed to handle the possibility that the variables might have different values one day, and not permitting overlapping matches would break that case.  My guess is that this was actually unintentional, so my solution is to rename all the variables to DBG_LOC and remove two of the above patterns.

S2. Superset before subset
--------------------------

For example, tools/clang/test/OpenMP/target_parallel_codegen.cpp has this:

```
// CHECK-DAG:   store i[[SZ]] {{4|8}}, i[[SZ]]* {{%[^,]+}}
...
// CHECK-DAG:   store i[[SZ]] 4, i[[SZ]]* {{%[^,]+}}
```

Some pattern X matches a superset of some pattern Y.  X appears before Y, but Y's intended match appears before X's intended match in the output.  The result is that X matches Y's intended match.  If overlapping is not permitted, Y has nothing to match, and FileCheck complains.  If overlapping is permitted, Y also matches Y's intended match, and X's intended match is quietly ignored.  This case is clearly unintentional.  One fix is to move X after Y.  A cleaner fix is to somehow make X distinct.

Another example of this scenario:

* test/Bitcode/thinlto-function-summary-originalnames.ll

S3. Wrong number of identical patterns
--------------------------------------

This scenario is just an incorrect attempt at the original use case I wanted to address.

For example, projects/openmp/runtime/test/ompt/parallel/not_enough_threads.c repeats the following three times:

```
// CHECK-DAG: {{^}}[[THREAD_ID:[0-9]+]]: ompt_event_implicit_task_begin: parallel_id=[[PARALLEL_ID]], task_id=[[IMPLICIT_TASK_ID:[0-9]+]]
// CHECK-DAG: {{^}}[[THREAD_ID]]: ompt_event_barrier_begin: parallel_id=[[PARALLEL_ID]], task_id=[[IMPLICIT_TASK_ID]]
```

The author was obviously expecting CHECK-DAG not to permit overlapping matches and wanted to check that the above lines appeared three times.  Specifically, he's expecting 3 threads after the master thread, but the run line sets OMP_THREAD_LIMIT=2.  If overlapping matches are permitted, this will permit any non-zero number of threads after the master thread.  If overlapping matches are not permitted, we must increase OMP_THREAD_LIMIT to at least 4 or reduce the number of pattern repetitions.

S4. CHECK-DAG unneeded
----------------------

For example, test/CodeGen/AMDGPU/store-v3i64.ll has:

```
; GCN-DAG: buffer_store_byte v
; GCN-DAG: buffer_store_byte v
; GCN-DAG: buffer_store_byte v
```

The intention appears to be the same as S3: match a pattern N times.  However, this example doesn't actually need CHECK-DAG at all.  First, all patterns in the group are the same, so they are not unordered, so CHECK would have been sufficient.  Second, there's only one occurrence in the actual output (which I'm assuming is correct), so there should be only one directive, so it really doesn't matter which of these directives we use.

S5. Incorrect pattern with same match
-------------------------------------

For example, projects/compiler-rt/test/profile/instrprof-visibility.cpp has:

```
// COV-DAG: {{.*}}|{{ *}}1|#ifndef NO_WEAK
...
// COV-DAG: {{.*}}|{{ *}}1|#ifndef NO_WEAK
```

The "1" does not appear in first occurrence in the actual output.  Let's assume the actual output is correct and the first pattern is incorrect.  If overlapping is permitted, the first pattern matches the intended line for the second pattern, which quietly matches that line too.  If overlapping is not permitted, FileCheck complains that the second pattern has nothing left to match.

An example where the second pattern is incorrect:

* test/Bitcode/thinlto-summary-local-5.0.ll

An example where the incorrect pattern is identical but it's not as obvious because a regex variable by a different name (ATOMIC_REDUCE_BARRIER_LOC instead of REDUCTION_LOC) is captured within it:

* tools/clang/test/OpenMP/for_reduction_codegen_UDR.cpp has:

Path Forward
------------------

These examples seem like strong evidence that permitting overlapping matches is a significant source of test errors.  Moreover, we have my original goal that some test authors were apparently assuming was already supported: to be able to handle sets of strings that are both unordered and non-unique.  Nevertheless, it's possible that there are use cases, such as the one suggested by S1, that we lose.

Here are some potential paths forward:

P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.

P2. Make CHECK-DAG non-overlapping but fix all test cases before committing that change.  Patch reviews will surely be slow.

P3. Make non-overlapping CHECK-DAG a special mode that's off by default until all test suites have been fixed.

By the way, I think it would be generally useful for FileCheck to have a debugging mode that prints every match.  It would be particularly useful while migrating to a non-overlapping CHECK-DAG.

Thoughts?

Joel

 
--paulr



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
I just stumbled across r332416, which was modifying a test with multiple
identical CHECK-DAG directives, which reminded me I needed to get back
to you about this... sorry for the slow response.

> From: Joel E. Denny [mailto:[hidden email]]
>> On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express
>>> that a pattern must have N non-overlapping matches.
>>
>> I think #1 is much more intuitive and easy to describe/document than #2.
>> Changing the meaning of DAG in that way is highly unlikely to affect any
>> existing test, IMO.  And if it does, my first expectation is that the test
>> wasn't doing what the author intended anyway.
>
> I implemented #1, and I now have 105 new unexpected tests failures from
> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1) test
> suites.

Okay... obviously more than I expected.  But hey, if we look at it as a
percentage of all tests, it's really small!  [attempting to salvage a
degree of self-respect]

> I've only looked at a few failures so far.  Of those, the problem is
> the expected one: in a CHECK-DAG group, multiple patterns match the
> same line, so not permitting overlapping matches causes some patterns
> never to match successfully.  However, all the overlapping matches so
> far seem unintentional.  That is, this new implementation seems to be
> catching test errors.

Awesome.  Catching test errors is a definite plus for the project.

> ...
> Path Forward
> ------------------
>
> These examples seem like strong evidence that permitting overlapping
> matches is a significant source of test errors.  Moreover, we have my
> original goal that some test authors were apparently assuming was
> already supported: to be able to handle sets of strings that are both
> unordered and non-unique.  Nevertheless, it's possible that there are
> use cases, such as the one suggested by S1, that we lose.

Hmmm... Either that one intended for the matches to be different, and
it's a bug that they aren't; or it's an overly flexible test, and
tightening it up does no harm.  The test change to accommodate the
updated FileCheck behavior would have to go through the original test
author or some other domain expert in order to determine which it is.

I don't see the loss of flexibility as necessarily a bad thing; although
we might find cases where behavior reasonably varies across targets, and
then we would need to work out how to accommodate those differences.

> Here are some potential paths forward:
>
> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
>
> P2. Make CHECK-DAG non-overlapping but fix all test cases before
> committing that change.  Patch reviews will surely be slow.
>
> P3. Make non-overlapping CHECK-DAG a special mode that's off by default
> until all test suites have been fixed.

I vote for P3.  It's easier to divide up the work if the feature is in
upstream FileCheck, and it's easy for someone doing the work to enable
the feature in their local branch.  I would not bother making it a
command-line switch, unless we really do come up with cases where the
overlapping matches are absolutely the right solution for some class of
tests.

I suggest you file a bug with the complete list of test failures that
non-overlapping CHECK-DAG finds.  That will help keep track of the work,
and if anyone else feels moved to pick up some part of it, they can make
notes in the bug so we can try to avoid duplicating effort.  Hopefully
I can devote some time to this myself, I think it's how CHECK-DAG should
have been done in the first place and I'm sad we didn't catch it at the
time.

> By the way, I think it would be generally useful for FileCheck to have
> a debugging mode that prints every match.  It would be particularly
> useful while migrating to a non-overlapping CHECK-DAG.
>
> Thoughts?
>
> Joel

A debugging mode like that sounds like a useful feature for writing a
test, independent of the migration aid.  Do it.
 
--paulr

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
Paul Robinson <[hidden email]> writes:

>> From: Joel E. Denny [mailto:[hidden email]]
>>> On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
>>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
>>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express
>>>> that a pattern must have N non-overlapping matches.
>>>
>>> I think #1 is much more intuitive and easy to describe/document than #2.
>>> Changing the meaning of DAG in that way is highly unlikely to affect any
>>> existing test, IMO.  And if it does, my first expectation is that the test
>>> wasn't doing what the author intended anyway.
>>
>> I implemented #1, and I now have 105 new unexpected tests failures from
>> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1) test
>> suites.
...

>> Here are some potential paths forward:
>>
>> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
>> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
>>
>> P2. Make CHECK-DAG non-overlapping but fix all test cases before
>> committing that change.  Patch reviews will surely be slow.
>>
>> P3. Make non-overlapping CHECK-DAG a special mode that's off by default
>> until all test suites have been fixed.
>
> I vote for P3.  It's easier to divide up the work if the feature is in
> upstream FileCheck, and it's easy for someone doing the work to enable
> the feature in their local branch.  I would not bother making it a
> command-line switch, unless we really do come up with cases where the
> overlapping matches are absolutely the right solution for some class of
> tests.

I feel like a variant of this where this mode is on by default would
make more sense. That is,

1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless
   --deprecated-allow-overlapping-check-dag or something is passed to
   FileCheck.
2. Add the flag to the 105 current test failures
3. File bugs and/or create reviews to remove the flag from each of the
   tests.

There are two reasons I prefer this approach. First, new tests are
"correct by default" while we clean up the existing problematic tests.
Second, out of tree work has an easy path forward and reasonable
forewarning. They can add the flag temporarily and fix problematic tests
when time permits, rather it suddenly breaking their CI whenever open
source flips the switch.

> I suggest you file a bug with the complete list of test failures that
> non-overlapping CHECK-DAG finds.  That will help keep track of the work,
> and if anyone else feels moved to pick up some part of it, they can make
> notes in the bug so we can try to avoid duplicating effort.  Hopefully
> I can devote some time to this myself, I think it's how CHECK-DAG should
> have been done in the first place and I'm sad we didn't catch it at the
> time.
>
>> By the way, I think it would be generally useful for FileCheck to have
>> a debugging mode that prints every match.  It would be particularly
>> useful while migrating to a non-overlapping CHECK-DAG.
>>
>> Thoughts?
>>
>> Joel
>
> A debugging mode like that sounds like a useful feature for writing a
> test, independent of the migration aid.  Do it.

+1
_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev


> -----Original Message-----
> From: Justin Bogner [mailto:[hidden email]] On Behalf Of Justin
> Bogner
> Sent: Wednesday, May 16, 2018 12:16 PM
> To: llvm-dev
> Cc: [hidden email]; Robinson, Paul
> Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
> occurrences of string
>
> Paul Robinson <[hidden email]> writes:
> >> From: Joel E. Denny [mailto:[hidden email]]
> >>> On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
> >>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> >>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to
> express
> >>>> that a pattern must have N non-overlapping matches.
> >>>
> >>> I think #1 is much more intuitive and easy to describe/document than
> #2.
> >>> Changing the meaning of DAG in that way is highly unlikely to affect
> any
> >>> existing test, IMO.  And if it does, my first expectation is that the
> test
> >>> wasn't doing what the author intended anyway.
> >>
> >> I implemented #1, and I now have 105 new unexpected tests failures from
> >> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1)
> test
> >> suites.
> ...
> >> Here are some potential paths forward:
> >>
> >> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
> >> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
> >>
> >> P2. Make CHECK-DAG non-overlapping but fix all test cases before
> >> committing that change.  Patch reviews will surely be slow.
> >>
> >> P3. Make non-overlapping CHECK-DAG a special mode that's off by default
> >> until all test suites have been fixed.
> >
> > I vote for P3.  It's easier to divide up the work if the feature is in
> > upstream FileCheck, and it's easy for someone doing the work to enable
> > the feature in their local branch.  I would not bother making it a
> > command-line switch, unless we really do come up with cases where the
> > overlapping matches are absolutely the right solution for some class of
> > tests.
>
> I feel like a variant of this where this mode is on by default would
> make more sense. That is,
>
> 1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless
>    --deprecated-allow-overlapping-check-dag or something is passed to
>    FileCheck.
> 2. Add the flag to the 105 current test failures
> 3. File bugs and/or create reviews to remove the flag from each of the
>    tests.
>
> There are two reasons I prefer this approach. First, new tests are
> "correct by default" while we clean up the existing problematic tests.
> Second, out of tree work has an easy path forward and reasonable
> forewarning. They can add the flag temporarily and fix problematic tests
> when time permits, rather it suddenly breaking their CI whenever open
> source flips the switch.

Those are excellent points, and that approach totally makes sense.
--paulr

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On Wed, May 16, 2018 at 11:40 AM, <[hidden email]> wrote:
I just stumbled across r332416, which was modifying a test with multiple
identical CHECK-DAG directives, which reminded me I needed to get back
to you about this... sorry for the slow response.

Not a problem.
 
> From: Joel E. Denny [mailto:[hidden email]]
>> On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express
>>> that a pattern must have N non-overlapping matches.
>>
>> I think #1 is much more intuitive and easy to describe/document than #2.
>> Changing the meaning of DAG in that way is highly unlikely to affect any
>> existing test, IMO.  And if it does, my first expectation is that the test
>> wasn't doing what the author intended anyway.
>
> I implemented #1, and I now have 105 new unexpected tests failures from
> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1) test
> suites.

Okay... obviously more than I expected.  But hey, if we look at it as a
percentage of all tests, it's really small!  [attempting to salvage a
degree of self-respect]

I didn't exactly expect it either.  :-)

> I've only looked at a few failures so far.  Of those, the problem is
> the expected one: in a CHECK-DAG group, multiple patterns match the
> same line, so not permitting overlapping matches causes some patterns
> never to match successfully.  However, all the overlapping matches so
> far seem unintentional.  That is, this new implementation seems to be
> catching test errors.

Awesome.  Catching test errors is a definite plus for the project.

Thanks for encouraging me to pursue this implementation.
 
> ...
> Path Forward
> ------------------
>
> These examples seem like strong evidence that permitting overlapping
> matches is a significant source of test errors.  Moreover, we have my
> original goal that some test authors were apparently assuming was
> already supported: to be able to handle sets of strings that are both
> unordered and non-unique.  Nevertheless, it's possible that there are
> use cases, such as the one suggested by S1, that we lose.

Hmmm... Either that one intended for the matches to be different, and
it's a bug that they aren't; or it's an overly flexible test, and
tightening it up does no harm.  The test change to accommodate the
updated FileCheck behavior would have to go through the original test
author or some other domain expert in order to determine which it is.

I don't see the loss of flexibility as necessarily a bad thing; although
we might find cases where behavior reasonably varies across targets, and
then we would need to work out how to accommodate those differences.

Agreed.

Thanks for the feedback.

Joel

> Here are some potential paths forward:
>
> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
>
> P2. Make CHECK-DAG non-overlapping but fix all test cases before
> committing that change.  Patch reviews will surely be slow.
>
> P3. Make non-overlapping CHECK-DAG a special mode that's off by default
> until all test suites have been fixed.

I vote for P3.  It's easier to divide up the work if the feature is in
upstream FileCheck, and it's easy for someone doing the work to enable
the feature in their local branch.  I would not bother making it a
command-line switch, unless we really do come up with cases where the
overlapping matches are absolutely the right solution for some class of
tests.

I suggest you file a bug with the complete list of test failures that
non-overlapping CHECK-DAG finds.  That will help keep track of the work,
and if anyone else feels moved to pick up some part of it, they can make
notes in the bug so we can try to avoid duplicating effort.  Hopefully
I can devote some time to this myself, I think it's how CHECK-DAG should
have been done in the first place and I'm sad we didn't catch it at the
time.

> By the way, I think it would be generally useful for FileCheck to have
> a debugging mode that prints every match.  It would be particularly
> useful while migrating to a non-overlapping CHECK-DAG.
>
> Thoughts?
>
> Joel

A debugging mode like that sounds like a useful feature for writing a
test, independent of the migration aid.  Do it.
 
--paulr



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
On Wed, May 16, 2018 at 12:24 PM, <[hidden email]> wrote:


> -----Original Message-----
> From: Justin Bogner [mailto:[hidden email]] On Behalf Of Justin
> Bogner
> Sent: Wednesday, May 16, 2018 12:16 PM
> To: llvm-dev
> Cc: [hidden email]; Robinson, Paul
> Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
> occurrences of string
>
> Paul Robinson <[hidden email]> writes:
> >> From: Joel E. Denny [mailto:[hidden email]]
> >>> On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
> >>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> >>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to
> express
> >>>> that a pattern must have N non-overlapping matches.
> >>>
> >>> I think #1 is much more intuitive and easy to describe/document than
> #2.
> >>> Changing the meaning of DAG in that way is highly unlikely to affect
> any
> >>> existing test, IMO.  And if it does, my first expectation is that the
> test
> >>> wasn't doing what the author intended anyway.
> >>
> >> I implemented #1, and I now have 105 new unexpected tests failures from
> >> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1)
> test
> >> suites.
> ...
> >> Here are some potential paths forward:
> >>
> >> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
> >> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
> >>
> >> P2. Make CHECK-DAG non-overlapping but fix all test cases before
> >> committing that change.  Patch reviews will surely be slow.
> >>
> >> P3. Make non-overlapping CHECK-DAG a special mode that's off by default
> >> until all test suites have been fixed.
> >
> > I vote for P3.  It's easier to divide up the work if the feature is in
> > upstream FileCheck, and it's easy for someone doing the work to enable
> > the feature in their local branch.  I would not bother making it a
> > command-line switch, unless we really do come up with cases where the
> > overlapping matches are absolutely the right solution for some class of
> > tests.
>
> I feel like a variant of this where this mode is on by default would
> make more sense. That is,
>
> 1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless
>    --deprecated-allow-overlapping-check-dag or something is passed to
>    FileCheck.
> 2. Add the flag to the 105 current test failures
> 3. File bugs and/or create reviews to remove the flag from each of the
>    tests.
>
> There are two reasons I prefer this approach. First, new tests are
> "correct by default" while we clean up the existing problematic tests.
> Second, out of tree work has an easy path forward and reasonable
> forewarning. They can add the flag temporarily and fix problematic tests
> when time permits, rather it suddenly breaking their CI whenever open
> source flips the switch.

Those are excellent points, and that approach totally makes sense.
--paulr


Make sense.  I'll work on putting the patch in phabricator and creating the bugzilla issue.  I'll get to implementing the debugging mode eventually.

Thanks.

Joel

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
On Wed, May 16, 2018 at 12:38 PM, Joel E. Denny <[hidden email]> wrote:
On Wed, May 16, 2018 at 12:24 PM, <[hidden email]> wrote:


> -----Original Message-----
> From: Justin Bogner [mailto:[hidden email]] On Behalf Of Justin
> Bogner
> Sent: Wednesday, May 16, 2018 12:16 PM
> To: llvm-dev
> Cc: [hidden email]; Robinson, Paul
> Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
> occurrences of string
>
> Paul Robinson <[hidden email]> writes:
> >> From: Joel E. Denny [mailto:[hidden email]]
> >>> On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
> >>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> >>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to
> express
> >>>> that a pattern must have N non-overlapping matches.
> >>>
> >>> I think #1 is much more intuitive and easy to describe/document than
> #2.
> >>> Changing the meaning of DAG in that way is highly unlikely to affect
> any
> >>> existing test, IMO.  And if it does, my first expectation is that the
> test
> >>> wasn't doing what the author intended anyway.
> >>
> >> I implemented #1, and I now have 105 new unexpected tests failures from
> >> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1)
> test
> >> suites.
> ...
> >> Here are some potential paths forward:
> >>
> >> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
> >> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
> >>
> >> P2. Make CHECK-DAG non-overlapping but fix all test cases before
> >> committing that change.  Patch reviews will surely be slow.
> >>
> >> P3. Make non-overlapping CHECK-DAG a special mode that's off by default
> >> until all test suites have been fixed.
> >
> > I vote for P3.  It's easier to divide up the work if the feature is in
> > upstream FileCheck, and it's easy for someone doing the work to enable
> > the feature in their local branch.  I would not bother making it a
> > command-line switch, unless we really do come up with cases where the
> > overlapping matches are absolutely the right solution for some class of
> > tests.
>
> I feel like a variant of this where this mode is on by default would
> make more sense. That is,
>
> 1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless
>    --deprecated-allow-overlapping-check-dag or something is passed to
>    FileCheck.
> 2. Add the flag to the 105 current test failures
> 3. File bugs and/or create reviews to remove the flag from each of the
>    tests.
>
> There are two reasons I prefer this approach. First, new tests are
> "correct by default" while we clean up the existing problematic tests.
> Second, out of tree work has an easy path forward and reasonable
> forewarning. They can add the flag temporarily and fix problematic tests
> when time permits, rather it suddenly breaking their CI whenever open
> source flips the switch.

Those are excellent points, and that approach totally makes sense.
--paulr


Make sense.  I'll work on putting the patch in phabricator and creating the bugzilla issue.  I'll get to implementing the debugging mode eventually.

Sorry for the delay.  It's in phabricator here:


The list of test failures has changed slightly.  I'm re-running and will create a bugzilla soon.

Thanks.

Joel


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

Robin Eklind via llvm-dev
On Sat, May 19, 2018 at 11:38 AM, Joel E. Denny <[hidden email]> wrote:
On Wed, May 16, 2018 at 12:38 PM, Joel E. Denny <[hidden email]> wrote:
On Wed, May 16, 2018 at 12:24 PM, <[hidden email]> wrote:


> -----Original Message-----
> From: Justin Bogner [mailto:[hidden email]] On Behalf Of Justin
> Bogner
> Sent: Wednesday, May 16, 2018 12:16 PM
> To: llvm-dev
> Cc: [hidden email]; Robinson, Paul
> Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
> occurrences of string
>
> Paul Robinson <[hidden email]> writes:
> >> From: Joel E. Denny [mailto:[hidden email]]
> >>> On Mon, May 7, 2018 at 12:56 PM, <[hidden email]> wrote:
> >>>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> >>>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to
> express
> >>>> that a pattern must have N non-overlapping matches.
> >>>
> >>> I think #1 is much more intuitive and easy to describe/document than
> #2.
> >>> Changing the meaning of DAG in that way is highly unlikely to affect
> any
> >>> existing test, IMO.  And if it does, my first expectation is that the
> test
> >>> wasn't doing what the author intended anyway.
> >>
> >> I implemented #1, and I now have 105 new unexpected tests failures from
> >> llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1)
> test
> >> suites.
> ...
> >> Here are some potential paths forward:
> >>
> >> P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
> >> #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
> >>
> >> P2. Make CHECK-DAG non-overlapping but fix all test cases before
> >> committing that change.  Patch reviews will surely be slow.
> >>
> >> P3. Make non-overlapping CHECK-DAG a special mode that's off by default
> >> until all test suites have been fixed.
> >
> > I vote for P3.  It's easier to divide up the work if the feature is in
> > upstream FileCheck, and it's easy for someone doing the work to enable
> > the feature in their local branch.  I would not bother making it a
> > command-line switch, unless we really do come up with cases where the
> > overlapping matches are absolutely the right solution for some class of
> > tests.
>
> I feel like a variant of this where this mode is on by default would
> make more sense. That is,
>
> 1. Modify CHECK-DAG so that overlapping patterns aren't allow, unless
>    --deprecated-allow-overlapping-check-dag or something is passed to
>    FileCheck.
> 2. Add the flag to the 105 current test failures
> 3. File bugs and/or create reviews to remove the flag from each of the
>    tests.
>
> There are two reasons I prefer this approach. First, new tests are
> "correct by default" while we clean up the existing problematic tests.
> Second, out of tree work has an easy path forward and reasonable
> forewarning. They can add the flag temporarily and fix problematic tests
> when time permits, rather it suddenly breaking their CI whenever open
> source flips the switch.

Those are excellent points, and that approach totally makes sense.
--paulr


Make sense.  I'll work on putting the patch in phabricator and creating the bugzilla issue.  I'll get to implementing the debugging mode eventually.

Sorry for the delay.  It's in phabricator here:


The list of test failures has changed slightly.  I'm re-running and will create a bugzilla soon.

The bugzilla is here:


The debugging mode patch is here:


Joel

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev