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

On Wed, May 16, 2018 at 12:24 PM, <paul.robinson at sony.com> wrote:
>
>
> > -----Original Message-----
> > From: Justin Bogner [mailto:justin at justinbogner.com] On Behalf Of
Justin
> > Bogner
> > Sent: Wednesday, May 16, 2018 12:16 PM
> > To: llvm-dev
> > Cc: jdenny.ornl at gmail.com; Robinson, Paul
> > Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
> > occurrences of string
> >
> > Paul Robinson <paul.robinson at sony.com> writes:
> > >> From: Joel E. Denny [mailto:jdenny.ornl at gmail.com]
> > >>> On Mon, May 7, 2018 at 12:56 PM, <paul.robinson at
sony.com> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180516/f6b3b84a/attachment.html>

On Wed, May 16, 2018 at 12:38 PM, Joel E. Denny <jdenny.ornl at gmail.com>
wrote:
> On Wed, May 16, 2018 at 12:24 PM, <paul.robinson at sony.com> wrote:
>
>>
>>
>> > -----Original Message-----
>> > From: Justin Bogner [mailto:justin at justinbogner.com] On Behalf
Of
>> Justin
>> > Bogner
>> > Sent: Wednesday, May 16, 2018 12:16 PM
>> > To: llvm-dev
>> > Cc: jdenny.ornl at gmail.com; Robinson, Paul
>> > Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple
>> > occurrences of string
>> >
>> > Paul Robinson <paul.robinson at sony.com> writes:
>> > >> From: Joel E. Denny [mailto:jdenny.ornl at gmail.com]
>> > >>> On Mon, May 7, 2018 at 12:56 PM, <paul.robinson at
sony.com> 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:

https://reviews.llvm.org/D47106

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

Thanks.

Joel
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180519/9beaff33/attachment.html>

On Sat, May 19, 2018 at 11:38 AM, Joel E. Denny <jdenny.ornl at gmail.com>
wrote:
> On Wed, May 16, 2018 at 12:38 PM, Joel E. Denny <jdenny.ornl at
gmail.com>
> wrote:
>
>> On Wed, May 16, 2018 at 12:24 PM, <paul.robinson at sony.com>
wrote:
>>
>>>
>>>
>>> > -----Original Message-----
>>> > From: Justin Bogner [mailto:justin at justinbogner.com] On
Behalf Of
>>> Justin
>>> > Bogner
>>> > Sent: Wednesday, May 16, 2018 12:16 PM
>>> > To: llvm-dev
>>> > Cc: jdenny.ornl at gmail.com; Robinson, Paul
>>> > Subject: Re: [llvm-dev] RFC: [FileCheck] CHECK-DAG for
multiple
>>> > occurrences of string
>>> >
>>> > Paul Robinson <paul.robinson at sony.com> writes:
>>> > >> From: Joel E. Denny [mailto:jdenny.ornl at gmail.com]
>>> > >>> On Mon, May 7, 2018 at 12:56 PM,
<paul.robinson at sony.com> 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:
>
> https://reviews.llvm.org/D47106
>
> The list of test failures has changed slightly.  I'm re-running and
will
> create a bugzilla soon.
>
The bugzilla is here:

https://bugs.llvm.org/show_bug.cgi?id=37532

The debugging mode patch is here:

https://reviews.llvm.org/D47114

Joel
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180519/c00f9286/attachment.html>