[llvm-dev] RFC: FileCheck Enhancements

[Elena Lepilkina via llvm-dev]

Hi everyone,

 

There was idea to add new directives to FileCheck:

1.       Directive to use some patterns as named template with or without parameters.

2.       CHECK-INCLUDE - Directive to include other file with checks to another.

3.       Expressions repeat  for CHECK - If statement should be checked several times repeat modifiers {n}, {n,m} , {,n}, {n,}, *, + can be used.

4.       Repeat in regexs - Repeat with current number should become available by using {n}, {n,m} , {,n}, {n,}

5.       CHECK-LABEL-DAG - Not sequential order of labels.

6.       Check statement for words only - // CHECK-WORD, // CHECK-WORD-NEXT, // CHECK-WORD-SAME, // CHECK-WORD-DAG, // CHECK-WORD-NOT.

7.       Wildcard for prefixes - If some statements should be checked regardless prefix, it should be used //{{*}}, //{{*}}-NEXT, //{{*}}-SAME and etc.

8.       Prefix with regular expressions - If statement should be checked if prefix matches some regular expression, it should be used {{regex}}:, {{regex}}-NEXT  and etc.

 

More information in file https://docs.google.com/document/d/1wAKNzU7-S2EeK1-aADwgP8dEiKfByKNazonybCQW3zs/edit?usp=sharing.

 

Now we have prototype with these features. It’s tested on LLVM 3.8.

There was found unsupported before directive in old test. Bug about this - https://llvm.org/bugs/show_bug.cgi?id=27852.

 

There is about 6% slowdown with new features when we tested them on 3.8.

 

I see that there are some changes in FileCheck LLVM 3.9 with new features too. We can publish patch for 3.8 and it can be adapted for LLVM 3.9. Is it interesting for anyone? And how will be better to publish patch as for 3.8 or for 3.9?

 

Thanks,

Elena.

[Jonathan Roelofs via llvm-dev]

On 5/24/16 7:51 AM, Elena Lepilkina via llvm-dev wrote:
> Hi everyone,
>
> There was idea to add new directives to FileCheck:
>

> 1.Directive to use some patterns as named template with or without
> parameters.
>
> 2.CHECK-INCLUDE - Directive to include other file with checks to another.
>
> 3.Expressions repeat for CHECK - If statement should be checked several

> times repeat modifiers {n}, {n,m} , {,n}, {n,}, *, + can be used.
>

> 4.Repeat in regexs - Repeat with current number should become available

> by using {n}, {n,m} , {,n}, {n,}
>

> 5.CHECK-LABEL-DAG - Not sequential order of labels.
>
> 6.Check statement for words only - // CHECK-WORD, // CHECK-WORD-NEXT, //

> CHECK-WORD-SAME, // CHECK-WORD-DAG, // CHECK-WORD-NOT.

What does this ^ do?

>
> 7.Wildcard for prefixes - If some statements should be checked

> regardless prefix, it should be used //{{*}}, //{{*}}-NEXT, //{{*}}-SAME
> and etc.

I'm not a fan of this ^ feature. I think it'll make testcases much
harder to understand.

>
> 8.Prefix with regular expressions - If statement should be checked if

> prefix matches some regular expression, it should be used {{regex}}:,
> {{regex}}-NEXT and etc.

I'm not a fan of this ^ feature. I think it'll make testcases much
harder to understand.

>
> More information in file
> https://docs.google.com/document/d/1wAKNzU7-S2EeK1-aADwgP8dEiKfByKNazonybCQW3zs/edit?usp=sharing.
>
> Now we have prototype with these features. It’s tested on LLVM 3.8.
>
> There was found unsupported before directive in old test. Bug about this
> - https://llvm.org/bugs/show_bug.cgi?id=27852.
>
> There is about 6% slowdown with new features when we tested them on 3.8.
>
> I see that there are some changes in FileCheck LLVM 3.9 with new
> features too. We can publish patch for 3.8 and it can be adapted for
> LLVM 3.9. Is it interesting for anyone? And how will be better to
> publish patch as for 3.8 or for 3.9?

Patches that apply on trunk are preferred (assuming the community
accepts these changes).

Jon

>
> Thanks,
>
> Elena.
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

--
Jon Roelofs
jona...@codesourcery.com
CodeSourcery / Mentor Embedded
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Elena Lepilkina via llvm-dev]

Hi,

CHECK-WORD - If you want find some string in file, but you want to be sure, that this string should be a separate word.

There are examples in file.
Prefixes which can be described as regular expressions should be turning on with option -regex-prefixes . By default, you can't use it.

Thanks for your comments.

[Jonathan Roelofs via llvm-dev]

On 5/24/16 8:10 AM, Elena Lepilkina wrote:
> Hi,
>
> CHECK-WORD - If you want find some string in file, but you want to be sure, that this string should be a separate word.

Is this functionally equivalent to doing:

// CHECK: {{\s}}whatever{{\s}}

Or is there some other subtlety about it?

Jon

[Elena Lepilkina via llvm-dev]

It's equivalent to {{\b}}whatever{{\b}}. I amn't sure if assertion \b is supported.
\s will not match with start and of line, but it should be matched.

Elena.

[Elena Lepilkina via llvm-dev]

Hi everyone,

I published patch for changes described in this RFC http://reviews.llvm.org/D20668.
But I don't know who can be reviewer for this patch.

Thanks,
Elena.

[Ehsan Amiri via llvm-dev]

7.       Wildcard for prefixes - If some statements should be checked regardless prefix, it should be used //{{*}}, //{{*}}-NEXT, //{{*}}-SAME and etc.

8.       Prefix with regular expressions - If statement should be checked if prefix matches some regular expression, it should be used {{regex}}:, {{regex}}-NEXT  and etc.

 

I, too, think wildcard and regular expression for prefixes will make it hard to read the test files. Currently I can highlight the prefix and focus on a specific test, but that won't be possible when these features are used. I prefer an easy to read but long test file to a hard to read but compact one.

[Ehsan Amiri via llvm-dev]

Technically that will be possible, but it will be time consuming to figure out which regular expressions should be highlighted, probability of mistake will be higher, etc.

 

[Elena Lepilkina via llvm-dev]

Okay, if this feature is undesirable by most of people, I can change published patch by removing this feature. But I would like to know opinion of people developing FileCheck before.

 

Elena.

 

From: Ehsan Amiri [mailto:ehsan...@gmail.com]
Sent: Thursday, May 26, 2016 5:35 PM
To: Elena Lepilkina <Elena.L...@synopsys.com>
Cc: llvm-dev <llvm...@lists.llvm.org>
Subject: Re: [llvm-dev] RFC: FileCheck Enhancements

 

7.       Wildcard for prefixes - If some statements should be checked regardless prefix, it should be used //{{*}}, //{{*}}-NEXT, //{{*}}-SAME and etc.

[James Y Knight via llvm-dev]

It's also an entirely unnecessary feature: you can use multiple --check-prefix arguments on the test run to accomplish the same thing, and many tests do that today. (e.g. "FileCheck --check-prefix=CHECK --check-prefix=SSE --check-prefix=SSE3").

[Elena Lepilkina via llvm-dev]

But then I should write

// CHECK: something

// SSE: something

// SSE3: something

 

With this feature it can be write // {{[A-Z0-9]+}} : something

 

From: James Y Knight [mailto:jykn...@google.com]
Sent: Thursday, May 26, 2016 5:53 PM
To: Ehsan Amiri <ehsan...@gmail.com>
Cc: Elena Lepilkina <Elena.L...@synopsys.com>; llvm-dev <llvm...@lists.llvm.org>
Subject: Re: [llvm-dev] RFC: FileCheck Enhancements

 

 

 

On Thu, May 26, 2016 at 10:35 AM, Ehsan Amiri via llvm-dev <llvm...@lists.llvm.org> wrote:

[Mehdi Amini via llvm-dev]

On May 26, 2016, at 8:01 AM, Elena Lepilkina via llvm-dev <llvm...@lists.llvm.org> wrote:

But then I should write

// CHECK: something

// SSE: something

// SSE3: something

I don't see why you need this? Why cant you only write:

// CHECK: something.

And all these will match it:

FileCheck

FileCheck --check-prefix=CHECK --check-prefix=SSE 

FileCheck --check-prefix=CHECK --check-prefix=SSE --check-prefix=SSE2

FileCheck --check-prefix=CHECK --check-prefix=SSE --check-prefix=SSE2 --check-prefix=SSE3

-- 

Mehdi

[Daniel Sanders via llvm-dev]

> 7.  Wildcard for prefixes - If some statements should be checked regardless prefix, it should be used //{{*}}, //{{*}}-NEXT, //{{*}}-SAME and etc.

 

For this one I agree that multiple check prefixes already provides this. The MIPS tests frequently have something like '--check-prefix=ALL --check-prefix=FOO' on one command and '—check-prefix=ALL –check-prefix=BAR'.

 

> 8.       Prefix with regular expressions - If statement should be checked if prefix matches some regular expression, it should be used {{regex}}:, {{regex}}-NEXT  and etc.

 

The previous example isn't very compelling but I can see how this feature could be useful to me. I have a number of tests that do something like:

                // O32:  something for O32

                // N32:  something for N32

                // N64:  something for N64

                // NEW:  something for both N32 and N64

But this is a bit clearer:

                // O32:  something for O32

                // N32:  something for N32

                // N64:  something for N64

                // {{N32|N64}}:  something for both N32 and N64

 

There's also some that define O32, O32EL, and O32EB which could drop the O32 and do:

                // {{O32(EL|EB)}}: any endian

                // {{O32(EL)}}: little endian

                // {{O32(EB)}}: big endian

In this example, I've included redundant parenthesis so that vim's '*' key can find me all the O32 lines, all the little endian lines, etc.

 

One last example is that I have some tests that define MIPS32R1, MIPS32R2, MIPS32R3, MIPS32R5, MIPS32R6 and MIPS64 equivalents of each.

                {{MIPS32R[2-5]}} would match MIPS32R2 through to MIPS32R6

                {{MIPS(32|64)R6}} would match MIPS32R6 and MIPS64R6

                {{MIPS64.*}} would match any MIPS64

This would remove a lot of redundancy but it's starting to harm readability. I'm not sure where I draw the line on that trade-off but I definitely wouldn't want complicated regexes.

[James Y Knight via llvm-dev]

For all of these changes: why/where are they actually useful?

Previous enhancements of FileCheck have mostly been added when needed by a test. It would greatly help your case for adding these new enhancements to show some tests which would be improved, or have greater clarity, by the use of these features.

I'm particularly skeptical about "3. Expressions repeat  for CHECK", "5. CHECK-LABEL-DAG", and (as noted before "7. Wildcard for prefixes" and "8. Prefix with regular expressions"

For "6. Check statement for words only" -- I think it might be better to just make that be the ONLY behavior, rather than an additional option -- if you intended to end a match in the middle of  a word, stick {{[^ ]*}} on it.

[Sergey Yakoushkin via llvm-dev]

Hi James, all,

> For all of these changes: why/where are they actually useful?

> Previous enhancements of FileCheck have mostly been added when needed by a test. It would greatly help your case for adding these new enhancements to show some tests which would be improved, or have greater clarity, by the use of these features.

Let me clarify. Elena's proposals are driven by development and testing of "downstream" commercial toolchain for embedded processors.

The proposed enhancements have practical use cases in internal code base. We did decide to upstream them.

However, we didn't look for representative examples in upstream repository (yet).

"3. Expressions repeat  for CHECK"

Tests containing duplicating CHECK lines: e.g. you want to match expression K times or maybe skip it to find specific expression.

Currently we have to duplicate CHECK or use grep and sed/awk scripts count patterns and then compare numbers later.

E.g. count number of LD instructions inside function or loop body. This extension can help to describe that as CHECK pattern.

"5. CHECK-LABEL-DAG"

CHECK-LABEL is used to annotate top-level entities, e.g. functions.

Suppose your tool can reorder output fragments (e.g. functions in ASM) comparing to original source file.

LABEL-DAG can help.

"8.       Prefix with regular expressions - If statement should be checked if prefix matches some regular expression, it should be used {{regex}}:, {{regex}}-NEXT  and etc. "

Our processors are highly configurable and we compile/apply same test for many ISA extensions options.

Image 100s variants of the same CHECK statements for various ISA configurations.

CHECK regexp prefixes can explain logic in compact and readable way, making it easier to understand 

Use cases are similar to Daniel's example for MIPS:

               {{MIPS32R[2-5]}} would match MIPS32R2 through to MIPS32R6

                {{MIPS(32|64)R6}} would match MIPS32R6 and MIPS64R6

                {{MIPS64.*}} would match any MIPS64

Thanks,

Sergey

[Robinson, Paul via llvm-dev]

> From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Elena Lepilkina via llvm-dev

> Sent: Tuesday, May 24, 2016 6:51 AM
> To: llvm-dev
> Subject: [llvm-dev] RFC: FileCheck Enhancements

>
> Hi everyone,
>
> There was idea to add new directives to FileCheck:
> 1.       Directive to use some patterns as named template with or without parameters.

Seems plausible, although I find the proposed syntax for a template call
with parameters to be very awkward.

> 2.       CHECK-INCLUDE - Directive to include other file with checks to another.
> 3.       Expressions repeat  for CHECK - If statement should be checked several times repeat modifiers {n}, {n,m} , {,n}, {n,}, *, + can be used.
> 4.       Repeat in regexs - Repeat with current number should become available by using {n}, {n,m} , {,n}, {n,}
> 5.       CHECK-LABEL-DAG - Not sequential order of labels.

After reading through the google doc I see what you are trying to do.

The motivation for CHECK-DAG is to reduce future test "churn" in case
someone makes a change that influences the output order of something,
but the order is not important to the test. The motivation is NOT to
support instability in the output order across runs of the same compiler.
Just so we're clear on that.

In principle I could see how the order of blocks or entire functions
could change over time, with no real relevance to a test, but how
necessary is it really? I have seen work go into Clang/LLVM over the
years to ensure stable output order, and my impression has been that
these changes typically have little complexity or performance effect
while significantly simplifying test effort.

> 6.       Check statement for words only - // CHECK-WORD, // CHECK-WORD-NEXT, // CHECK-WORD-SAME, // CHECK-WORD-DAG, // CHECK-WORD-NOT.

I would expect a regex package to provide a word-break meta symbol,
in which case this feature would be redundant. I admit I have not
checked whether the package we use has this feature, or how it would
be spelled.

> 7.       Wildcard for prefixes - If some statements should be checked regardless prefix, it should be used //{{*}}, //{{*}}-NEXT, //{{*}}-SAME and etc.
> 8.       Prefix with regular expressions - If statement should be checked if prefix matches some regular expression, it should be used {{regex}}:, {{regex}}-NEXT  and etc.
>
> More information in file https://docs.google.com/document/d/1wAKNzU7-S2EeK1-aADwgP8dEiKfByKNazonybCQW3zs/edit?usp=sharing.

I noticed the google doc stated that multi-line patterns are not
supported. That's not actually the case, although it's a bit obscure:
the [[:space:]] character-class will match EOL and allow you to write
a multi-line CHECK.

>
> Now we have prototype with these features. It's tested on LLVM 3.8.
> There was found unsupported before directive in old test. Bug about this - https://llvm.org/bugs/show_bug.cgi?id=27852.
>
> There is about 6% slowdown with new features when we tested them on 3.8.

Is that a 6% slowdown just in FileCheck, or when running the entire
Clang/LLVM test suite? Making the entire test run 6% more expensive
seems like a lot.
--paulr

>
> I see that there are some changes in FileCheck LLVM 3.9 with new features too. We can publish patch for 3.8 and it can be adapted for LLVM 3.9. Is it interesting for anyone? And how will be better to publish patch as for 3.8 or for 3.9?
>
> Thanks,
> Elena.

[Elena Lepilkina via llvm-dev]

Hi Paul,

Thank you for information about the [[:space:]] character-class.
About performance I tested on Clang/LLVM test suite. I try to profile and the problem is that I used regular expressions a lot for supporting some new features and functions in your regex library are very slow . Regex library is very old and quite awkward, in my opinion.
May be you will see some ways to improve performance, if some features are decided to include in LLVM FileCheck and I publish patches for each feature separately(I saw your comment in published patch).
I see that there are a lot of opinions, may be it will be better to vote. I suggest vote in file https://docs.google.com/spreadsheets/d/1p8Hi_PH3Nd2kEtYveCwKXENmJGOzYWW6us0XK7eVvOw/edit?usp=sharing. There will be history and it's possible to check that nobody votes twice. Then somebody can stop voting at one moment and I will be able to understand what patches I should do and publish if there are some changes you would like.

Thanks,
Elena.

-----Original Message-----
From: Robinson, Paul [mailto:paul.r...@sony.com]
Sent: Friday, May 27, 2016 2:26 AM
To: Elena Lepilkina <Elena.L...@synopsys.com>; llvm...@lists.llvm.org
Subject: RE: [llvm-dev] RFC: FileCheck Enhancements

[Elena Lepilkina via llvm-dev]

Hi all,

Voting may be a bad idea. I' ll be glad to hear more opinions here and may be some suggestions how to improve new features (may be there are ideas how template descriptions can be done simplier). After I try to accept your ideas and opinions to make new FileCheck features better.
There was an idea to change regex library for support assertion \b. Are there any real plans to change regex library?
I will make changes and publish separate patches a month later, because I'll take a holiday in June.

[Jonathan Roelofs via llvm-dev]

On 5/26/16 5:25 PM, Robinson, Paul via llvm-dev wrote:
>
>> 7. Wildcard for prefixes - If some statements should be checked regardless prefix, it should be used //{{*}}, //{{*}}-NEXT, //{{*}}-SAME and etc.
>> 8. Prefix with regular expressions - If statement should be checked if prefix matches some regular expression, it should be used {{regex}}:, {{regex}}-NEXT and etc.
>>
>> More information in file https://docs.google.com/document/d/1wAKNzU7-S2EeK1-aADwgP8dEiKfByKNazonybCQW3zs/edit?usp=sharing.
>
> I noticed the google doc stated that multi-line patterns are not
> supported. That's not actually the case, although it's a bit obscure:
> the [[:space:]] character-class will match EOL and allow you to write
> a multi-line CHECK.

Also: CHECK-SAME.

Jon

--
Jon Roelofs
jona...@codesourcery.com
CodeSourcery / Mentor Embedded

[Jonathan Roelofs via llvm-dev]

On 5/27/16 7:41 AM, Jonathan Roelofs via llvm-dev wrote:
>
>
> On 5/26/16 5:25 PM, Robinson, Paul via llvm-dev wrote:
>>
>>> 7. Wildcard for prefixes - If some statements should be checked
>>> regardless prefix, it should be used //{{*}}, //{{*}}-NEXT,
>>> //{{*}}-SAME and etc.
>>> 8. Prefix with regular expressions - If statement should be
>>> checked if prefix matches some regular expression, it should be used
>>> {{regex}}:, {{regex}}-NEXT and etc.
>>>
>>> More information in file
>>> https://docs.google.com/document/d/1wAKNzU7-S2EeK1-aADwgP8dEiKfByKNazonybCQW3zs/edit?usp=sharing.
>>>
>>
>> I noticed the google doc stated that multi-line patterns are not
>> supported. That's not actually the case, although it's a bit obscure:
>> the [[:space:]] character-class will match EOL and allow you to write
>> a multi-line CHECK.
>
> Also: CHECK-SAME.

Whoops... I misread... Ignore me.

Jon

[Elena Lepilkina via llvm-dev]

Hi all,

I' ll be glad to hear more opinions and may be some suggestions how to improve new features (may be there are ideas how template descriptions can be done simplier). After we try to accept your ideas and opinions to make new FileCheck features better.

I will make changes and publish separate patches a month later, because I'll take a holiday in June.

Thanks,
Elena.

-----Original Message-----
From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Elena Lepilkina via llvm-dev

Sent: Friday, May 27, 2016 12:09 PM
To: Robinson, Paul <paul.r...@sony.com>; llvm...@lists.llvm.org

[Michael Kruse via llvm-dev]

2016-05-26 18:13 GMT+02:00 James Y Knight via llvm-dev
<llvm...@lists.llvm.org>:

> For "6. Check statement for words only" -- I think it might be better to
> just make that be the ONLY behavior, rather than an additional option -- if
> you intended to end a match in the middle of a word, stick {{[^ ]*}} on it.

I support this. I found it very confusing when
; CHECK-NEXT: br
to check for a branch instruction also matched a line eg.
%2 = add i32 %subregion, i32 5

However, it is a breaking change and therefore might require changes
in existing tests.

Michael

[Robinson, Paul via llvm-dev]

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of
> Michael Kruse via llvm-dev
> Sent: Friday, June 03, 2016 11:03 AM
> To: James Y Knight
> Cc: llvm-dev
> Subject: Re: [llvm-dev] RFC: FileCheck Enhancements
>

> 2016-05-26 18:13 GMT+02:00 James Y Knight via llvm-dev
> <llvm...@lists.llvm.org>:
> > For "6. Check statement for words only" -- I think it might be better to
> > just make that be the ONLY behavior, rather than an additional option --
> if
> > you intended to end a match in the middle of a word, stick {{[^ ]*}} on
> it.
>
> I support this. I found it very confusing when
> ; CHECK-NEXT: br
> to check for a branch instruction also matched a line eg.
> %2 = add i32 %subregion, i32 5
>
> However, it is a breaking change and therefore might require changes
> in existing tests.

Definitely. I've seen things like "CHECK: lea" which is intentionally
insensitive to "leal" versus "leaq" as the generated X86 instruction.
It would take a while to find all such tests and make them tolerate a
word-based FileCheck.
--paulr

[Elena Lepilkina via llvm-dev]

Hi all,

Now all discussed enhancements are divided into separate patches. Moreover I have found mistake which reduces performance. Now it's fixed.

List of patches (for those interested):
Repeats in regular expressions - https://reviews.llvm.org/D22342
Including files - https://reviews.llvm.org/D22344
Expressions repeat for CHECK and CHECK-NEXT - https://reviews.llvm.org/D22345
CHECK-LABEL-DAG - https://reviews.llvm.org/D22348
CHECK-WORD - https://reviews.llvm.org/D22353
prefixes-regular expressions - https://reviews.llvm.org/D22401
pattern templates - https://reviews.llvm.org/D22403.

[Robinson, Paul via llvm-dev]

Please add llvm-commits as a subscriber to all of these reviews
so everyone has a chance to see/comment on them.
Thanks,
--paulr

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Elena
> Lepilkina via llvm-dev
> Sent: Friday, July 15, 2016 6:05 AM
> To: llvm-dev
> Subject: Re: [llvm-dev] RFC: FileCheck Enhancements
>

[Nemanja Ivanovic via llvm-dev]

Hi Elena,

Sorry if my question is something you've previously addressed. I didn't comment on the reviews themselves as I don't have any expertise with the implementation of FileCheck. However, much like most LLVM developers, I actively use the tool.

A brief look at the patches does not show updates to the documentation. I'm sure that many LLVM developers like myself will gladly use new features if they prove useful, but we just need to know about them. Rather than the source, I personally get information about how to use it from here: http://llvm.org/docs/CommandGuide/FileCheck.html.

So I would say that for most of us, if the feature is not documented there, it is as if it doesn't exist.

Is there a separate patch (or a plan to put one up for review) for the documentation updates?

Thanks,

Nemanja

[Mehdi Amini via llvm-dev]

> On Jul 15, 2016, at 8:58 AM, Robinson, Paul via llvm-dev <llvm...@lists.llvm.org> wrote:
>
> Please add llvm-commits as a subscriber to all of these reviews
> so everyone has a chance to see/comment on them.

Adding llvm-commits after the fact ends up in the mailing list not archiving the context of the reviews.
If these patches are fresh (no significant review occurred), they should be closed and new revisions need to be opened, with llvm-commits as a subscriber from the start.

—
Mehdi

[Vedant Kumar via llvm-dev]

> On Jul 15, 2016, at 5:36 PM, Mehdi Amini via llvm-dev <llvm...@lists.llvm.org> wrote:
>
>> On Jul 15, 2016, at 8:58 AM, Robinson, Paul via llvm-dev <llvm...@lists.llvm.org> wrote:
>>
>> Please add llvm-commits as a subscriber to all of these reviews
>> so everyone has a chance to see/comment on them.
>
> Adding llvm-commits after the fact ends up in the mailing list not archiving the context of the reviews.
> If these patches are fresh (no significant review occurred), they should be closed and new revisions need to be opened, with llvm-commits as a subscriber from the start.

Ah, oops. I've started reviews of a few of the patches. If you plan on re-creating the reviews, please mark the relevant in-line comments on these as "Done" so it's easier to track what's changed: D22353, D22403, D22344.

thanks
vedant

[Elena Lepilkina via llvm-dev]

Hi,

 

I thought documentation will be changed after review if some changes are accepted. But as I need to create new patches because of next comments, I’ll add changes in documentation.

 

Thanks,

Elena.

 

From: Nemanja Ivanovic [mailto:nemanj...@gmail.com]

Sent: Friday, July 15, 2016 7:19 PM
To: Robinson, Paul <paul.r...@sony.com>

[James Y Knight via llvm-dev]

It does send a patch to the list if you upload a new diff to the existing review after adding the list as a subscriber. That seems the best way to go usually?

[Elena Lepilkina via llvm-dev]

Thank you, James.

 

Then I’ll update  all patches.

[David Blaikie via llvm-dev]

It still doesn't include the original context/description of the patch, though. Yes, you can copy/paste that in, though.

[Mehdi Amini via llvm-dev]

We had a long thread about that a few weeks (months?) ago: the conclusion (as I remember) was roughly a guideline to “always start a new revision to have a proper mailing-list thread starting with context (i.e. patch description)”

(and my dissident minority opinion that it is only worth it if there hasn’t been significant round of reviews going on on the existing revision)

— 

Mehdi

[Dean Michael Berris via llvm-dev]

> On 19 Jul 2016, at 04:18, Mehdi Amini via llvm-dev <llvm...@lists.llvm.org> wrote:
>
> We had a long thread about that a few weeks (months?) ago: the conclusion (as I remember) was roughly a guideline to “always start a new revision to have a proper mailing-list thread starting with context (i.e. patch description)”
> (and my dissident minority opinion that it is only worth it if there hasn’t been significant round of reviews going on on the existing revision)
>

Pardon me for missing that discussion, this may have already been asked before: but is it possible to make arcanist default subscribe the correct commits mailing list in the process? This should make it at least harder to forget.

Cheers

[Mehdi Amini via llvm-dev]

> On Jul 18, 2016, at 8:53 PM, Dean Michael Berris <dean....@gmail.com> wrote:
>
>
>> On 19 Jul 2016, at 04:18, Mehdi Amini via llvm-dev <llvm...@lists.llvm.org> wrote:
>>
>> We had a long thread about that a few weeks (months?) ago: the conclusion (as I remember) was roughly a guideline to “always start a new revision to have a proper mailing-list thread starting with context (i.e. patch description)”
>> (and my dissident minority opinion that it is only worth it if there hasn’t been significant round of reviews going on on the existing revision)
>>
>
> Pardon me for missing that discussion, this may have already been asked before: but is it possible to make arcanist default subscribe the correct commits mailing list in the process? This should make it at least harder to forget.

It seems certainly possible, and has been asked before as well, and my memory of the answer is along the line of “patch welcome”.
(I.e. some community member are spending time to customize and maintain our fabricator instance, but they have some real work to do as well, so we’re resource limited).

—
Mehdi

[Dean Michael Berris via llvm-dev]

> On 19 Jul 2016, at 14:09, Mehdi Amini <mehdi...@apple.com> wrote:
>
>
>> On Jul 18, 2016, at 8:53 PM, Dean Michael Berris <dean....@gmail.com> wrote:
>>
>>
>>> On 19 Jul 2016, at 04:18, Mehdi Amini via llvm-dev <llvm...@lists.llvm.org> wrote:
>>>
>>> We had a long thread about that a few weeks (months?) ago: the conclusion (as I remember) was roughly a guideline to “always start a new revision to have a proper mailing-list thread starting with context (i.e. patch description)”
>>> (and my dissident minority opinion that it is only worth it if there hasn’t been significant round of reviews going on on the existing revision)
>>>
>>
>> Pardon me for missing that discussion, this may have already been asked before: but is it possible to make arcanist default subscribe the correct commits mailing list in the process? This should make it at least harder to forget.
>
> It seems certainly possible, and has been asked before as well, and my memory of the answer is along the line of “patch welcome”.
> (I.e. some community member are spending time to customize and maintain our fabricator instance, but they have some real work to do as well, so we’re resource limited).
>

Cool, thanks!

PS. I did some searching for this, and it seems there's no "easy" way to do this customisation via the project config. Probably something that has to be done on the server side. (See https://secure.phabricator.com/Q268).

[Elena Lepilkina via llvm-dev]

Hi all,

I made new patches for most of changes with llvm-commits subscriber. But two patches were updated, because there are a lot of comments (patch for CHECK-WORD and patch for templates pattern). Will it be ok?

Thanks, Elena.

-----Original Message-----
From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Dean Michael Berris via llvm-dev
Sent: Tuesday, July 19, 2016 6:53 AM
To: Mehdi Amini <mehdi...@apple.com>

Cc: via llvm-dev <llvm...@lists.llvm.org>
Subject: Re: [llvm-dev] RFC: FileCheck Enhancements

[Elena Lepilkina via llvm-dev]

And I also want to ask again about possible change of regexp library. There are a lot of comments that some changes in FileCheck are useless, because they may be replaced by using some features of regular expressions, but they are not supported by current library.

I don't know a lot about modern C++ regexp library, but there are:
1. PCRE(pcre.h)
2. std::regex. It has no necessary features. So it can't be taken.
3. There is regex library in boost.
4. Also there is regex library in poco.

Are there any other variants?

[James Y Knight via llvm-dev]

I had actually just meant in my comments to update the library we have, not switch to a brand new one. 

[Elena Lepilkina via llvm-dev]

But as I understood current library is quite old and it is branched from OpenBSD library, and there are no new versions.  

Do you suggest to make changes in this version?

[Vedant Kumar via llvm-dev]

Hi Elena,

> On Jul 19, 2016, at 6:36 AM, Elena Lepilkina via llvm-dev <llvm...@lists.llvm.org> wrote:
>
> Hi all,
>
> I made new patches for most of changes with llvm-commits subscriber. But two patches were updated, because there are a lot of comments (patch for CHECK-WORD and patch for templates pattern). Will it be ok?

IMO it's fine to keep some of the original reviews if you don't want to
discard/recreate their state.

Please list the most up-to-date set of Phab URL's here, with a little note next
to the ones which did not initially CC llvm-commits.

Thanks again for working on this!

vedant

[Elena Lepilkina via llvm-dev]

List of last patches:

1. FileCheck Enhancement - CHECK-WORD (llvm-commits was added later as diff update) - https://reviews.llvm.org/D22353
2. FileCheck Enhancement - pattern templates(llvm-commits was added later as diff update) - https://reviews.llvm.org/D22403
3. FileCheck Enhancement - repeats in regular expressions (new review with llvm-commits) - https://reviews.llvm.org/D22454
4. FileCheck Enhancement - Including files (new review with llvm-commits) - https://reviews.llvm.org/D22500
5. FileCheck Enhancement - Expressions repeat for CHECK and CHECK-NEXT (new review with llvm-commits) - https://reviews.llvm.org/D22501
6. FileCheck Enhancement - CHECK-LABEL-DAG (new review with llvm-commits) - https://reviews.llvm.org/D22502
7. FileCheck Enhancement - prefixes-regular expressions (new review with llvm-commits) - https://reviews.llvm.org/D22503

Thanks,
Elena.

-----Original Message-----
From: v...@apple.com [mailto:v...@apple.com]
Sent: Tuesday, July 19, 2016 8:42 PM
To: Elena Lepilkina <Elena.L...@synopsys.com>

[Elena Lepilkina via llvm-dev]

Hi all,

Some discussions and comments were made in reviews. Much time has already passed since last comment and uploading changed patches. I made small summary report about features here, because there are some doubts about syntax of some features and changes in patches and it'll be great to know more opinions.

1. FileCheck Enhancement - CHECK-WORD (https://reviews.llvm.org/D22353)
I replace special directives by flag --check-word, which turns on mode for each directive in file. It's obvious that this mode can be replaced using \b assert, but current regexp library doesn't have support of this assert and I have no answer to question about possibility of change current library.
There was the discussion about that such mode can be made default, but there were doubts about necessity of a lot of work for changing existing tests.
And I made experiment which proves that a lot of old tests will be failed with such mode on.
Expected Passes : 15810
Expected Failures : 125
Unsupported Tests : 195
Unexpected Passes : 4
Unexpected Failures: 1128

2. FileCheck Enhancement - pattern templates ( https://reviews.llvm.org/D22403)
There are some doubts about syntax of templates. I agree that use of \#, \:, \= is quite different from usual characters in FileCheck and was chosen because of same approach for escaping in regexp. Adrian Prantl suggested to use double-brackets "[[" to escape.
Old syntax:
\#(template_name) - use of template 'template_name'. It can occur in CHECK-PATTERN line, when description of one template includes other templates described before. (Without quote, I don't know how escape # here)
\:(Variable_name)- template variable with name 'variable_name'
\:(variable_name)\=(value) - current value of template variable(it's needed when you use template with variables).
Suggested new syntax:
[[#template_name]] - use of template 'template_name'. It can occur in CHECK-PATTERN line, when description of one template includes other templates described before. (Without quote, I don't know how escape # here)
[[:Variable_name]] - template variable with name 'variable_name'
[[:variable_name=value]] - current value of template variable(it's needed when you use template with variables).
It'll be great to hear more opinions and suggestions about syntax. May be someone has really good ideas. Then I'll be able to change it.


3. FileCheck Enhancement - repeats in regular expressions (https://reviews.llvm.org/D22454), FileCheck Enhancement - Including files (https://reviews.llvm.org/D22500), FileCheck Enhancement - Expressions repeat for CHECK and CHECK-NEXT(https://reviews.llvm.org/D22501), FileCheck Enhancement - CHECK-LABEL-DAG(https://reviews.llvm.org/D22502), FileCheck Enhancement - prefixes-regular expressions (https://reviews.llvm.org/D22503)
There were no comments about these enhancements at all. Your opinions are very important.

I hope that some of these changes will be useful for FileCheck users, so I need your opinions to get opportunity for review to be resumed.

Thanks, Elena.

-----Original Message-----
From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Elena Lepilkina via llvm-dev

[Vedant Kumar via llvm-dev]

> On Aug 24, 2016, at 2:04 AM, Elena Lepilkina <Elena.L...@synopsys.com> wrote:
>
> Hi all,
>
> Some discussions and comments were made in reviews. Much time has already passed since last comment and uploading changed patches. I made small summary report about features here, because there are some doubts about syntax of some features and changes in patches and it'll be great to know more opinions.
>
> 1. FileCheck Enhancement - CHECK-WORD (https://reviews.llvm.org/D22353)
> I replace special directives by flag --check-word, which turns on mode for each directive in file. It's obvious that this mode can be replaced using \b assert, but current regexp library doesn't have support of this assert and I have no answer to question about possibility of change current library.
> There was the discussion about that such mode can be made default, but there were doubts about necessity of a lot of work for changing existing tests.
> And I made experiment which proves that a lot of old tests will be failed with such mode on.
> Expected Passes : 15810
> Expected Failures : 125
> Unsupported Tests : 195
> Unexpected Passes : 4
> Unexpected Failures: 1128

I would rather not introduce churn in our tests by turning on --check-word by
default. I'm also not convinced that turning on --check-word at the test level
is the right move: having a CHECK-WORD directive is more flexible, and not a
serious inconvenience (as compared to writing "CHECK").

>
> 2. FileCheck Enhancement - pattern templates ( https://reviews.llvm.org/D22403)
> There are some doubts about syntax of templates. I agree that use of \#, \:, \= is quite different from usual characters in FileCheck and was chosen because of same approach for escaping in regexp. Adrian Prantl suggested to use double-brackets "[[" to escape.
> Old syntax:
> \#(template_name) - use of template 'template_name'. It can occur in CHECK-PATTERN line, when description of one template includes other templates described before. (Without quote, I don't know how escape # here)
> \:(Variable_name)- template variable with name 'variable_name'
> \:(variable_name)\=(value) - current value of template variable(it's needed when you use template with variables).
> Suggested new syntax:
> [[#template_name]] - use of template 'template_name'. It can occur in CHECK-PATTERN line, when description of one template includes other templates described before. (Without quote, I don't know how escape # here)
> [[:Variable_name]] - template variable with name 'variable_name'
> [[:variable_name=value]] - current value of template variable(it's needed when you use template with variables).
> It'll be great to hear more opinions and suggestions about syntax. May be someone has really good ideas. Then I'll be able to change it.

First, I want to recap the FileCheck workflow Elena is proposing:

1. Define patterns using the CHECK-DEFINE-PATTERN directive. Defined patterns
have a name and may optionally have parameters.

2. Use defined patterns in the usual CHECK* directives.

This is similar to how FileCheck patterns work already. The difference is
that the patterns are defined using a dedicated directive, *not* when the
pattern is first encountered. E.g, here is what you can do today:

// RUN: echo "%r1 %r2" | FileCheck %s
// CHECK: %[[register:[a-z]+]]1
// CHECK-SAME: %[[register]]2

With the proposed changes, we'll be able to write something like:

// RUN: echo "%cmp %cmp" | FileCheck %s
// CHECK-DEFINE-PATERN: register(n): {{[a-z]+}}n
// CHECK: %[[register("1")]]
// CHECK-SAME: %[[register("2")]]

I saw "something like" because we haven't decided on the syntax for defining
and using patterns (that's what this thread is for). Briefly, here's the syntax
I'd like to use:

// Defining patterns.
CHECK-DEFINE-PATERN: <Name>(<Ident>, ...)?: <Pattern>

Where <Pattern> is a list of <PatternElement>, and a <PatternElement> is
either a regex ('{{' POSIX_REGEX '}}') or an argument identifier (IDENT).

// Using patterns.
CHECK: [[<Name>(<Argument>, ...)?]]

Fleshing this out some more, here is my candidate grammar (see the end of this
email for the current grammar):

ACTION <- CHECK ':' MATCH '\n' ;
ACTION <- CHECK-DEFINE-PATTERN ':' IDENT PARAMLIST? ':' PATTERN_ELEMENT* '\n' ;
PARAMLIST <- '(' IDENT (',' IDENT)* ')' ;
PATTERN_ELEMENT <- IDENT ;
PATTERN_ELEMENT <- REGEX ;
MATCH <- ;
MATCH <- TEXT MATCH ;
MATCH <- REGEX MATCH ;
MATCH <- VAR MATCH ;
REGEX <- '{{' POSIX_REGEX '}}' ;
VAR <- '[[' IDENT ':' POSIX_REGEX ']]' ;
VAR <- '[[' IDENT ARGLIST? ']]' ;
ARGLIST <- '(' ARG (',' ARG)* ')' ;
ARG <- "([^"]|\\")*" ;

> 3. FileCheck Enhancement - repeats in regular expressions (https://reviews.llvm.org/D22454), FileCheck Enhancement - Including files (https://reviews.llvm.org/D22500), FileCheck Enhancement - Expressions repeat for CHECK and CHECK-NEXT(https://reviews.llvm.org/D22501), FileCheck Enhancement - CHECK-LABEL-DAG(https://reviews.llvm.org/D22502), FileCheck Enhancement - prefixes-regular expressions (https://reviews.llvm.org/D22503)
> There were no comments about these enhancements at all. Your opinions are very important.

I personally am waiting for some version of D22403 to land in-tree before
starting on the other reviews. This would help me gauge what others in the
community are thinking and what they need.

>
> I hope that some of these changes will be useful for FileCheck users, so I need your opinions to get opportunity for review to be resumed.

thanks,
vedant

Original FileCheck grammar (shamelessly copied from the grammar Adrian posted
to D22403):

ACTION <- CHECK ':' MATCH '\n' ;
MATCH <- ;
MATCH <- TEXT MATCH ;
MATCH <- REGEX MATCH ;
MATCH <- VAR MATCH ;
REGEX <- '{{' POSIX_REGEX '}}' ;
VAR <- '[[' IDENT ':' POSIX_REGEX ']]' ;
VAR <- '[[' IDENT ']]' ;

[Elena Lepilkina via llvm-dev]

Hi Vedant,

Thank you for your detailed answer. There was such idea with brackets for template parameters after I made patch. I think this is a good idea. Question is only if empty brackets are necessary when pattern is without parameters.

I have doubts about syntax of pattern usage. I think that '?' at the end will be inconspicuous. Moreover this is not similar to current syntax in FileCheck. Metacharacters are usually used at the beginning, aren't they? Why not then describe usage as ?[[<pattern>]] ?

One advantages of current syntax is that if pattern is used inside regular expression there is no need to interrupt regular expression and then start it again. But with new syntax I will need to write something like this {{regex_part1}}[[<pattern>?]]{{regex_part2}}.

[Elena Lepilkina via llvm-dev]

CHECK-DEFINE-PATERN: <Name>(<Ident>, ...)?: <Pattern>

And it's also interesting your opinion about variables names usage during describing pattern in <Pattern> part.

[Mehdi Amini via llvm-dev]

At first I thought that `register(n)` was some sort of macro, but if it is suppose to be equivalent to the example above of what we do “today”, then using “register(“1”)” is supposed to “capture” the ‘r’ part of the register on the first match.

So you cannot reuse “register()” later to capture another expression. For instance:

 // RUN: FileCheck %s

 // CHECK-DEFINE-PATERN: register(n): {{[a-z]+}}n
 // CHECK: %[[register("1")]]
 // CHECK-SAME: %[[register("2")]]

 // CHECK: %[[register("1")]]
 // CHECK-SAME: %[[register("2")]]

%r1 %r2

%reg1 %reg2 #will fail here.

If true, I find this confusing, if not, I missed something in your example.

— 

Mehdi

[Vedant Kumar via llvm-dev]

> On Aug 31, 2016, at 5:41 AM, Elena Lepilkina <Elena.L...@synopsys.com> wrote:
>
> Hi Vedant,
>
> Thank you for your detailed answer. There was such idea with brackets for template parameters after I made patch. I think this is a good idea. Question is only if empty brackets are necessary when pattern is without parameters.

I'd rather drop the parens when there are no parameters, but I have no hard
arguments for why that's a better approach. I'll post a follow-up to Mehdi's
email with a suggestion for how to disambiguate pattern usages from normal
matches.

> I have doubts about syntax of pattern usage. I think that '?' at the end will be inconspicuous.

The '?' just means: "the preceding bit of the grammar is optional". It isn't a
literal '?'. So, '[[' IDENT ARGLIST? ']]' would match both '[[' IDENT ']]' and
'[[' IDENT ARGLIST ']]'. Maybe I'm misunderstanding -- did you mean that having
an optional ARGLIST is the real issue?

> Moreover this is not similar to current syntax in FileCheck. Metacharacters are usually used at the beginning, aren't they? Why not then describe usage as ?[[<pattern>]] ?
>
> One advantages of current syntax is that if pattern is used inside regular expression there is no need to interrupt regular expression and then start it again. But with new syntax I will need to write something like this {{regex_part1}}[[<pattern>?]]{{regex_part2}}.

Yes. But, I don't view this as a disadvantage of the new syntax. IMO this is a
bit easier to read.

thanks,

[Vedant Kumar via llvm-dev]

> At first I thought that `register(n)` was some sort of macro, but if it is suppose to be equivalent to the example above of what we do “today”, then using “register(“1”)” is supposed to “capture” the ‘r’ part of the register on the first match.

This is a problem with my suggested gramamr (specifically: it doesn't provide a
way to use defined patterns to capture text for later reference). Fred brought
up that he was confused by this too.

One way to fix it is to use '@' before using patterns. I'll recap the suggested
grammar and work through another example. Here's how you'd define a pattern:

CHECK-DEFINE-PATTERN: car(make, model, year): {{Found a }} make model {{, from }} year

And here's how you'd use it, *without* capturing the text into a variable:

CHECK: [[@car("Honda", "Accord", "2009")]]

(Note, this matches the text: "Found a Honda Accord, from 2009".)

To use a pattern and capture the matched text in "MY_CAR", you'd write:

CHECK: [[MY_CAR @car("Honda", "Accord", "2009")]]

This gives us an unambiguous way to capture text matched via a defined pattern,
which is visually distinct from how normal regex-based capturing works.

Note that subsequent uses of "MY_CAR" should work as expected (i.e, you can do
'[[MY_CAR]]' later in the test).

Revised grammar:

ACTION <- CHECK ':' MATCH '\n' ;
ACTION <- CHECK-DEFINE-PATTERN ':' IDENT PARAMLIST? ':' PATTERN_ELEMENT* '\n' ;
PARAMLIST <- '(' IDENT (',' IDENT)* ')' ;

PATTERN_ELEMENT <- IDENT | REGEX;
MATCH <- (TEXT | REGEX | PATTERN_USE | VAR)* ;
REGEX <- '{{' POSIX_REGEX '}}' ;
PATTERN_USE <- '[[' '@' IDENT ']]' ;
VAR <- '[[' IDENT ':' POSIX_REGEX ']]' ;
VAR <- '[[' IDENT '@' IDENT ARGLIST? ']]' ;

ARGLIST <- '(' ARG (',' ARG)* ')' ;
ARG <- "([^"]|\\")*" ;

vedant

[Mehdi Amini via llvm-dev]

Thanks Vedant, that answers perfectly my concern!

—
Mehdi

[Elena Lepilkina via llvm-dev]

Yes, I now understand what you suggested.

> CHECK-DEFINE-PATTERN: car(make, model, year): {{Found a }} make model {{, from }} year

But I think that in pattern I should show that I use parameter. I thought that patterns can also be simple strings. All strings should be regexs in pattern and parameters can't be used in pattern in your example. But I want to use parameters in regexs. For example, {{Found a \#make{2,4}}}. I need to know if this parameter name or simple text. So I asked you before about syntax of parameters usage during describing patterns.

[Elena Lepilkina via llvm-dev]

About CHECK-WORD. There is no problem to return first patch with CHECK-WORD, CHECK-WORD-SAME and etc.

I added option because there were a lot of opinions that add a lot of new directives is a bad idea.

 

Thanks,

Elena.

[Elena Lepilkina via llvm-dev]

I wanted to start making some change. But I thought and I don't understand why it's necessary to add @.

// RUN: FileCheck %s
// CHECK-DEFINE-PATERN: register(n): {{[a-z]+}}n
// CHECK: %[[register("1")]]
// CHECK-SAME: %[[register("2")]]
// CHECK: %[[register("1")]]
// CHECK-SAME: %[[register("2")]]

This example will be equivalent to

// RUN: FileCheck %s
// CHECK-DEFINE-PATERN: register(n): {{[a-z]+}}n

// CHECK: %{{[a-z]+}}1
// CHECK-SAME: %{{[a-z]+}}2
// CHECK: %{{[a-z]+}}1
// CHECK-SAME: %{{[a-z]+}}2

Now we have another syntax, but pattern work such way.

If it's necessary to use pattern in variables, we can made
[[REGISTER:%[[register(1)]]]].

May be it is more useful to change extra brackets with @, but in Mehdi's example everything will be okay id I understood well.

Thanks,
Elena.

-----Original Message-----
From: v...@apple.com [mailto:v...@apple.com]

Sent: Wednesday, August 31, 2016 10:57 PM

[Vedant Kumar via llvm-dev]

> On Aug 31, 2016, at 11:26 PM, Elena Lepilkina <Elena.L...@synopsys.com> wrote:
>
> Yes, I now understand what you suggested.
>
>> CHECK-DEFINE-PATTERN: car(make, model, year): {{Found a }} make model {{, from }} year
>
> But I think that in pattern I should show that I use parameter. I thought that patterns can also be simple strings. All strings should be regexs in pattern and parameters can't be used in pattern in your example. But I want to use parameters in regexs. For example, {{Found a \#make{2,4}}}. I need to know if this parameter name or simple text. So I asked you before about syntax of parameters usage during describing patterns.

With the syntax I'm suggesting, there is a clear distinction between parameters
and regexes inside of pattern definitions. This is really valuable to me (and
probably others): it means that I don't have to learn an alternate,
FileCheck-specific flavor of regexes. That could also make it easier to switch
to a different regex engine in the future.

The tradeoff here is that 'simple strings' inside of pattern definitions need
to be wrapped in '{{' '}}', i.e they are actually just regexes. We'd also need
logic to escape metacharacters in pattern arguments. IMO, this is a fair
tradeoff. Note that you can still apply regexes to parameters, like this:

CHECK-DEFINE-PATTERN: one_or_more(x): x {{+}}

Wdyt?

[Vedant Kumar via llvm-dev]

It is necessary to be able to capture a match made with a pattern in a
variable. The syntax you suggested for doing this with your example has a
problem: it's ambiguous. Consider:

[[REGISTER:%[[register(1)]]]]

Is this saying that we should match a literal '%' character, followed by a
literal '[' character etc.? Or, is it a pattern application? There's no way to
disambiguate the two.

That is the role of the '@' character in my suggested grammar. It makes the
grammar unambiguous, and it makes it easy to notice pattern applications. So:

[[REGISTER @register("1")]]

and

[[REGISTER: register]]]

Are two totally different things, and it should be obvious that they are
different.

vedant

[Sergey Yakoushkin via llvm-dev]

Hi,

I also like proposed grammar. One minor correction, I guess you want to allow pattern use with parameters as well:

 PATTERN_USE <- '[[' '@' IDENT ARGLIST? ']]' ;

[Elena Lepilkina via llvm-dev]

Ok, when I change syntax I will load new patch.

[Vedant Kumar via llvm-dev]

Thanks Elena!

> Sergey wrote:
>

> I also like proposed grammar. One minor correction, I guess you want to allow pattern use with parameters as well:
>
> PATTERN_USE <- '[[' '@' IDENT ARGLIST? ']]' ;

Thanks, that looks better!

vedant

[Elena Lepilkina via llvm-dev]

New patch for patterns with new syntax was uploaded to Phabricator.

[Elena Lepilkina via llvm-dev]

Hi,

I have question again about modifiers for pattern parameters.

Vedant suggested such way.

> CHECK-DEFINE-PATTERN: one_or_more(x): x {{+}}

But I have some doubts. This should be equal to x+. This approach differs from standard one.
In FileCheck I can write

CHECK: {{x|y}}{{something}}

This line will be equal to regex (x|y)(something).

But if I use suggested approach and write same string in pattern CHECK-DEFINE-PATTERN: example: {{x|y}}{{something}}, it will be equal to (x|ysomething).
As user I expected behavior as in first check.

Thanks, Elena.

[Vedant Kumar via llvm-dev]

That's a good example. While '{{REGEX}}' usually creates a new matching group,
we could introduce some new syntax to make it possible to use pattern arguments
inside of regexes. E.g for an argument named 'x', writing 'x' in a pattern
definition preserves the current behavior, and writing '#x' concatenates the
value of 'x' with any surrounding regexes (resulting in just one matching
group).

So, to match one or more things you could write:

CHECK-DEFINE-PATTERN: one_or_more(x): #x {{+}}
CHECK: [[@one_or_more("1")]]

Without the '#', you'd see a syntax error because '+' isn't a valid regex.

I like this approach because it doesn't require changing the definitions of
REGEX or POSIX_REGEX. It'd be interesting to hear what other people think.

We should be able to hash this out in parallel to the work on D22403, since the
plan is to defer work on pattern arguments until basic support for pattern
definitions has landed.

Revised grammar (** proposal **):

ACTION <- CHECK ':' MATCH '\n' ;
ACTION <- CHECK-DEFINE-PATTERN ':' IDENT PARAMLIST? ':' PATTERN_ELEMENT* '\n' ;
PARAMLIST <- '(' IDENT (',' IDENT)* ')' ;

PATTERN_ELEMENT <- '#'? IDENT ;
PATTERN_ELEMENT <- REGEX ;

MATCH <- (TEXT | REGEX | PATTERN_USE | VAR)* ;
REGEX <- '{{' POSIX_REGEX '}}' ;

PATTERN_USE <- '[[' '@' IDENT ARGLIST? ']]' ;

VAR <- '[[' IDENT ':' POSIX_REGEX ']]' ;
VAR <- '[[' IDENT '@' IDENT ARGLIST? ']]' ;
ARGLIST <- '(' ARG (',' ARG)* ')' ;
ARG <- "([^"]|\\")*" ;

best,
vedant

[Chris Lattner via llvm-dev]

> ARG <- "([^"]|\\")*” ;

This seems like a really complicated set of extensions to filecheck for marginal gain. I’m not enthused by the idea that you’d have to actually utter this in every individual test that needs to use the feature, because that would lead to boilerplate.

Instead of doing this, has anyone considered baking “modes” into Filecheck to support the important clients (e.g. LLVM IR, MC, clang, etc)? This would mean that a test could just add a “--mode=llvmir” flag to filecheck and get a bunch of baked in patterns, potentially with magic syntax. Something like this would avoid having to redundantly enter "%[0-9]+” a kajillion times all over the place.

-Chris

[Elena Lepilkina via llvm-dev]

As I understood, I should do first of all patch with patterns implementation only with simple parameters without modifiers? And after add this feature separately?

-----Original Message-----
From: v...@apple.com [mailto:v...@apple.com]
Sent: Tuesday, September 13, 2016 4:05 AM
To: Elena Lepilkina <Elena.L...@synopsys.com>
Cc: Mehdi Amini <mehdi...@apple.com>; llvm...@lists.llvm.org
Subject: Re: [llvm-dev] RFC: FileCheck Enhancements

[Elena Lepilkina via llvm-dev]

Hi,

There is including directive CHECK-INCLUDE as one of implemented enhancement. Using it you can include file with defined patterns. There is a good idea to get some such files in open source and may be adding options is simplier then choosing file for including. But including files is more flexible, in my opinion.
Now this feature has no feedback and I'll be glad to hear some opinions about it. https://reviews.llvm.org/D22500
Pattern is only first step.

Thank you for your suggestion.

Thanks, Elena.

-----Original Message-----
From: Chris Lattner [mailto:clat...@apple.com]
Sent: Tuesday, September 13, 2016 9:22 AM
To: Vedant Kumar <v...@apple.com>
Cc: Elena Lepilkina <Elena.L...@synopsys.com>; llvm...@lists.llvm.org
Subject: Re: [llvm-dev] RFC: FileCheck Enhancements

[Vedant Kumar via llvm-dev]

I hadn't considered adding modes to FileCheck. That seems like it could work
pretty well.

Even if that's the route we're going to take, I'd prefer that the FileCheck
modes simply expose pre-defined patterns instead of introducing magic syntax.
It would be a shame to have to hack FileCheck to use check patterns for clients
that aren't LLVM IR, clang, etc.

Having check patterns available would let me simplify some of my tests, making
them easier to update. I hope that we can find a minimal/unintrusive way of
adding at least a basic version of this feature. But, you're right that we
shouldn't do it unless it's generally useful.

vedant

[Chris Lattner via llvm-dev]

On Sep 13, 2016, at 11:16 AM, Vedant Kumar <v...@apple.com> wrote:
>> Instead of doing this, has anyone considered baking “modes” into Filecheck to support the important clients (e.g. LLVM IR, MC, clang, etc)? This would mean that a test could just add a “--mode=llvmir” flag to filecheck and get a bunch of baked in patterns, potentially with magic syntax. Something like this would avoid having to redundantly enter "%[0-9]+” a kajillion times all over the place.
>
> I hadn't considered adding modes to FileCheck. That seems like it could work
> pretty well.
>
> Even if that's the route we're going to take, I'd prefer that the FileCheck
> modes simply expose pre-defined patterns instead of introducing magic syntax.
> It would be a shame to have to hack FileCheck to use check patterns for clients
> that aren't LLVM IR, clang, etc.

Why?

This is a tradeoff: the more extensible you make filecheck (by introducing ever more complicated DSLs into it) the more esoteric it gets, and the less likely that someone will be able to actually UNDERSTAND a random filecheck invocation that they come across.

The advantage of adding a few hard coded modes to filecheck is that we can have specific standardized solutions to important use cases that the LLVM project has. FileCheck lives in service of LLVM and its downstream projects, it shouldn’t aspire to be some general tool that needs a DSL. If a downstream project outside of the LLVM project (e.g. the Rust compiler) wanted its own mode, I think it would be fine to add it, and having a gaggle of modes seems preferable to me than building in a complex DSL for describing named patterns.

[Vedant Kumar via llvm-dev]

Let me start by saying that adding a few hard-coded patterns to FileCheck could
work well for many of llvm's tests. I'd be perfectly fine with that approach.

My thinking was that if it isn't easy to teach FileCheck about new patterns,
it's less likely that people will do it. I'm not interested in making FileCheck
a general-purpose testing tool, but I would like to use custom patterns to
simplify in-tree tests that aren't in *.ll or *.s files.

I don't think that all extensions to the FileCheck syntax would cause serious
maintainability issues. Being able to define patterns in some tests could
actually make them more readable. However, if there isn't a way to extend
FileCheck without creating serious readability issues, I agree that we
shouldn't do it.

vedant

[Chris Lattner via llvm-dev]

On Sep 13, 2016, at 8:14 PM, Vedant Kumar <v...@apple.com> wrote:
>
> Let me start by saying that adding a few hard-coded patterns to FileCheck could
> work well for many of llvm's tests. I'd be perfectly fine with that approach.
>
> My thinking was that if it isn't easy to teach FileCheck about new patterns,
> it's less likely that people will do it. I'm not interested in making FileCheck
> a general-purpose testing tool, but I would like to use custom patterns to
> simplify in-tree tests that aren't in *.ll or *.s files.
>
> I don't think that all extensions to the FileCheck syntax would cause serious
> maintainability issues. Being able to define patterns in some tests could
> actually make them more readable. However, if there isn't a way to extend
> FileCheck without creating serious readability issues, I agree that we
> shouldn't do it.

Ok, it sounds like you agree that modes would be a good way to solve the most common case. How about we start with that. After that lands and gets adopted, we can see how big the middle zone served by custom pattern is, and what we are leaving on the table by not supporting it.

-Chris

[Mehdi Amini via llvm-dev]

> On Sep 14, 2016, at 3:10 PM, Chris Lattner via llvm-dev <llvm...@lists.llvm.org> wrote:
>
> On Sep 13, 2016, at 8:14 PM, Vedant Kumar <v...@apple.com> wrote:
>>
>> Let me start by saying that adding a few hard-coded patterns to FileCheck could
>> work well for many of llvm's tests. I'd be perfectly fine with that approach.
>>
>> My thinking was that if it isn't easy to teach FileCheck about new patterns,
>> it's less likely that people will do it. I'm not interested in making FileCheck
>> a general-purpose testing tool, but I would like to use custom patterns to
>> simplify in-tree tests that aren't in *.ll or *.s files.
>>
>> I don't think that all extensions to the FileCheck syntax would cause serious
>> maintainability issues. Being able to define patterns in some tests could
>> actually make them more readable. However, if there isn't a way to extend
>> FileCheck without creating serious readability issues, I agree that we
>> shouldn't do it.
>
> Ok, it sounds like you agree that modes would be a good way to solve the most common case. How about we start with that. After that lands and gets adopted, we can see how big the middle zone served by custom pattern is, and what we are leaving on the table by not supporting it.

How you would define the “modes” without a DSL? Hardcoding them inside FileCheck itself?
It seems that it would make the bar higher to add new pattern, and lock-us with this particular hard-coded solution (i.e. moving to a DSL solution after the fact would be harder).

On the opposite, I would expect that having the “DSL” support would make it easier to create/define these “modes”, even if we don’t use the “advanced DSL" in the tests themselves (we may even be able to forbid the use of these patterns outside of “mode file” if it is a strong concern).
I tend to think that with a low entry bar to create a “mode file”, it may be easier to have for example a “mode file” that defines patterns for some target-specific register format, and include it only in the tests for this particular target.

—
Mehdi

[Chris Lattner via llvm-dev]

> On Sep 14, 2016, at 3:23 PM, Mehdi Amini <mehdi...@apple.com> wrote:
>
>
>> On Sep 14, 2016, at 3:10 PM, Chris Lattner via llvm-dev <llvm...@lists.llvm.org> wrote:
>>
>> On Sep 13, 2016, at 8:14 PM, Vedant Kumar <v...@apple.com> wrote:
>>>
>>> Let me start by saying that adding a few hard-coded patterns to FileCheck could
>>> work well for many of llvm's tests. I'd be perfectly fine with that approach.
>>>
>>> My thinking was that if it isn't easy to teach FileCheck about new patterns,
>>> it's less likely that people will do it. I'm not interested in making FileCheck
>>> a general-purpose testing tool, but I would like to use custom patterns to
>>> simplify in-tree tests that aren't in *.ll or *.s files.
>>>
>>> I don't think that all extensions to the FileCheck syntax would cause serious
>>> maintainability issues. Being able to define patterns in some tests could
>>> actually make them more readable. However, if there isn't a way to extend
>>> FileCheck without creating serious readability issues, I agree that we
>>> shouldn't do it.
>>
>> Ok, it sounds like you agree that modes would be a good way to solve the most common case. How about we start with that. After that lands and gets adopted, we can see how big the middle zone served by custom pattern is, and what we are leaving on the table by not supporting it.
>
> How you would define the “modes” without a DSL? Hardcoding them inside FileCheck itself?

Yes.

> It seems that it would make the bar higher to add new pattern,

How so? It is as easy to change filecheck as it is to change a testcase, at least in the llvm repo.

> and lock-us with this particular hard-coded solution (i.e. moving to a DSL solution after the fact would be harder).

I don’t see how. The uses of filecheck in practice are very stratified based on what is being tested (e.g. llc output vs .ll files), I think that < 12 modes would cover the vast majority of the cases we care about.

> On the opposite, I would expect that having the “DSL” support would make it easier to create/define these “modes”, even if we don’t use the “advanced DSL" in the tests themselves (we may even be able to forbid the use of these patterns outside of “mode file” if it is a strong concern).

Right, but this is exactly what I *don’t* want. I don’t want it to be “too easy” to create micro DSLs, I want the bar to be somewhat higher so people use fewer better modes, rather than geeking out and getting crazy with a custom DSL that is only used in one or two tests.

Most people encounter a random variety of tests based on when they break them in their development. We don’t want them to have to spend an excessive amount of time to understand “how the test works” before they are able to dig into “what they did that broke the test”. Fewer but better modes is good this way.

-Chris

[Elena Lepilkina via llvm-dev]

Excuse me, but as I understood mode will have predefined named patterns set.
Then each mode will have a lot of patterns. Do you think that it's better for users to remember a lot of pattern names?
When new user reads test with usage of such pattern he needs to find its meaning in documentation. There isn't very easy for new user understand tests. Moreover, there is already hardcoded LINE variable. Suggested syntax of using patterns is very similar to using LINE. If patterns will be hardcoded then it's difficult to see difference between LINE and pattern. Moreover, FileCheck is quite complicated now. Understanding tests for new user is difficult process. I don't like reading FileCheck source code if I don't know how this pattern works exactly.

I understood that you are afraid of chaos in tests. But I can create terrible test for reading even with current syntax. All FileCheck users are programmers, and if they want they will add patterns in FileCheck source code. I think nobody will use patterns without necessity if there is such syntax in FileCheck.

I'm not against modes with predefined patterns, but I think that patterns can do some test easily to read by removing repeats of big regular expressions.

Thanks, Elena.

-----Original Message-----
From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Chris Lattner via llvm-dev
Sent: Thursday, September 15, 2016 1:38 AM
To: Mehdi Amini <mehdi...@apple.com>

Cc: llvm...@lists.llvm.org
Subject: Re: [llvm-dev] RFC: FileCheck Enhancements

[Robinson, Paul via llvm-dev]

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Elena
> Lepilkina via llvm-dev
> Sent: Thursday, September 15, 2016 12:26 AM
> To: Chris Lattner; Mehdi Amini
> Cc: llvm...@lists.llvm.org
> Subject: Re: [llvm-dev] RFC: FileCheck Enhancements
>

> Excuse me, but as I understood mode will have predefined named patterns
> set.
> Then each mode will have a lot of patterns. Do you think that it's better
> for users to remember a lot of pattern names?

My understanding is that you intended patterns to be used in conjunction
with INCLUDE, and that combination is much the same as modes with
predefined patterns. The main difference is where to find the list of
patterns: in the included file, or in FileCheck documentation.

> When new user reads test with usage of such pattern he needs to find its
> meaning in documentation. There isn't very easy for new user understand
> tests. Moreover, there is already hardcoded LINE variable. Suggested
> syntax of using patterns is very similar to using LINE. If patterns will
> be hardcoded then it's difficult to see difference between LINE and
> pattern. Moreover, FileCheck is quite complicated now. Understanding tests
> for new user is difficult process. I don't like reading FileCheck source
> code if I don't know how this pattern works exactly.
>
> I understood that you are afraid of chaos in tests. But I can create
> terrible test for reading even with current syntax. All FileCheck users
> are programmers, and if they want they will add patterns in FileCheck
> source code. I think nobody will use patterns without necessity if there
> is such syntax in FileCheck.

People will use a feature even when it is not necessary, if they believe
it will make the test easier to read.

For example, lots of tests use the -SAME feature even when it is not
necessary; unfortunately this means the semantics of the test change subtly,
and in a way that the inventor of the -SAME feature did not intend. This
can cause tests to fail (or pass!) unexpectedly. Fixing this problem could
mean surveying all tests that use -SAME to make sure the problem does not
occur (in the current set of tests), or we can modify FileCheck to fix the
problem for all current and future tests.

If we discover some common issue with patterns in the future, it will be
easier to fix it for all tests if the patterns are predefined by modes.
--paulr

>
> I'm not against modes with predefined patterns, but I think that patterns
> can do some test easily to read by removing repeats of big regular
> expressions.
>
> Thanks, Elena.
>
>
> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev...@lists.llvm.org] On Behalf Of Chris
> Lattner via llvm-dev
> Sent: Thursday, September 15, 2016 1:38 AM
> To: Mehdi Amini <mehdi...@apple.com>
> Cc: llvm...@lists.llvm.org
> Subject: Re: [llvm-dev] RFC: FileCheck Enhancements
>
>
> > On Sep 14, 2016, at 3:23 PM, Mehdi Amini <mehdi...@apple.com> wrote:
> >
> >

> >> On Sep 14, 2016, at 3:10 PM, Chris Lattner via llvm-dev <llvm-