[RFC][FileCheck] Improving input dump readability

PR #198138 makes a significant change that will surely be pervasive in the LLVM development experience. I believe it strictly improves FileCheck’s output by addressing some readability issues that have been discussed for years.

The main purpose of this RFC is to announce the change so that its sudden appearance does not confuse people. However, if it turns out people feel the change will introduce new problems for their use cases, then this RFC is also an opportunity to iterate further or object before the PR lands.

The examples below are based on main at c2047f919e7549c2853b609b4cbef9d0adb9c3af.

Example: Reducing Verbosity from X~~

For this example, I introduced a mistake into the second FileCheck directive of llvm/test/Transforms/LoopUnroll/epilog_const_phi.ll. Without this PR, FileCheck then produces this input dump:

With this PR, FileCheck instead produces this input dump:

In general, this PR changes FileCheck to never emit X~~~~, which is a verbose way to depict the search range for an unmatched pattern. Instead, FileCheck emits annotations depicting just the start and end of that search range. The PR’s initial comment discusses details like the rationale for using exclusive bounds to depict the search range.

The most recent observation of this verbosity that I recall appears in issue #77257. Thanks to @Shivam, @MaskRay, and others who drove that discussion.

Example: Adding search ranges for all errors

For this example, I introduced a mistake into the third FileCheck directive of llvm/test/Transforms/LoopUnroll/expensive-tripcount.ll. With this PR, FileCheck then produces this input dump:

Without this PR, input lines 1-9 and 92-97 are not shown. However, lines 1-9 are highly relevant to the actual problem because that is where the CHECK-NEXT directive was expected to match but did not.

In general, this PR adds annotations depicting the search range for any failed FileCheck pattern. The PR initial comment discusses why that is useful generally and points to additional examples.

Conclusion

I hope people agree that PR #198138 is strictly an improvement to FileCheck’s input dumps. If so, we can land it quickly. The PR has already been accepted (thanks go to @MaskRay for all the reviews). I plan to wait 1 week for replies to this RFC before landing, but I can wait longer if people need more time.

In the past, people have proposed entirely new formats for FileCheck’s output, as in the issue cited above. Such ideas can be valuable, and I do not wish to discourage their investigation. However, to avoid confusion, I respectfully ask that this RFC’s discussion stay focused on whether PR #198138 is a helpful incremental improvement for FileCheck’s current output format.

8 Likes

To be honest I don’t think I ever made the connection between all the ~ and the search range, I always worked it out in my head somehow. So +1 for the labels, and I like the reduced noise when trying to read it.

In the example on the PR the “search range start” seems to be closer to the {. Is the idea to push the “search range…” labels to the right, or have them next to the {}?

Also at first glance it seems more logical to say no match found and then { search range start. The search range is a detail of the failure to match, as I understand it.

I see in the PR example you have instead match on wrong line, that one it makes sense to be within the range so I can see where the wrong line actually is.

Which was the case before in fact, so this is not a blocker for your changes but if you want another improvement there’s an idea.

It sort of works for the old style because the error is printed on the same line as the start of the range, where the new style puts it on a new line which is more “inside” the range visually.

Thanks for your feedback.

With or without this PR, notes like “search range start”, “error: no match found”, etc. always start after the last column of the input line. Especially when an input line has many annotations with markers (^~~~, {, etc.) in different columns and possibly has some annotations without markers, this alignment offers a nice general balance between simplicity and readability, in my opinion.

Yes, I understand, for this (admittedly common) case in isolation. But when I consider the behavior more generally, it seems more consistent to me if all of an error’s annotations appear after the search range start. Examples below.

Yes, that is a good example of when we have no choice. Another is matched unexpected patterns (e.g., CHECK-NOT):

<<<<<<
         1: hello
not:2'0         {   search range start (exclusive)
         2: world
not:2'1     !~~~~   error: no match expected
         3: again
not:2'2           } search range end (exclusive)
>>>>>>

When we do have a choice and there are many associated annotations, here is an example of what this PR does now:

<<<<<<
              1: def-match1 def-match2
check:2'0                            {   search range start (exclusive)
check:2'1                                error: match failed for invalid pattern
check:2'2                                undefined variable: UNDEF
check:2'3                                with "DEF_MATCH1" equal to "def-match1"
check:2'4                                with "DEF_NOMATCH" equal to "foobar"
              2: def-match1 def-nomatch
check:2'5                 ?               possible intended match
check:2'6                               } search range end (exclusive)
>>>>>>

If we want to move the error annotation before the search range, we then have the question of whether to move all the other annotations before too, except of course the fuzzy match cannot move. Here is what it looks like when moving the others:

<<<<<<
              1: def-match1 def-match2
check:2'0                                error: match failed for invalid pattern
check:2'1                                undefined variable: UNDEF
check:2'2                                with "DEF_MATCH1" equal to "def-match1"
check:2'3                                with "DEF_NOMATCH" equal to "foobar"
check:2'4                            {   search range start (exclusive)
              2: def-match1 def-nomatch
check:2'5                 ?               possible intended match
check:2'6                               } search range end (exclusive)
>>>>>>

Perhaps I have been looking at the former form too long, but the initial annotations in the latter form look strange to me:

  • They seem to have no anchor in the input text. Why does FileCheck place them on this input line? Actually, it’s the search range that determines their placement. From the perspective of FileCheck’s matching behavior, the search range is always computed first.
  • Some annotations are before the search range start while one must be after.
  • I guess I have also become accustomed to the notion that any error occurs within the search range.

Mostly, I just prefer the simple consistency of having all of an error’s annotations appear after the search range start.

But I can be outvoted.

Thanks. It would be nice to land this first and make that tweak later if my arguments above are not convincing.

To emphasize the consistency issue further, imagine if hello and world appear on the same line. Should we do the following?

<<<<<<
         1: hello world 
not:2'0           !~~~~   error: no match expected
not:2'1         {         search range start (exclusive)
         2: again 
not:2'2           } search range end (exclusive)
>>>>>>

Having seen your later examples I agree with you for all the rest of the errors, but I think it’s still easy to look at:

output line
    { range starts here
    error: no match found

And think the error applies to the line not to the range.

But then again, this has always been a “no match in what exactly?” sort of error. If we start printing the ranges like you propose, it can say “no match found in range”, the range is marked directly above, and it’s much easier to understand.

output line
    { range starts here
    error: no match found in range

Appending “in range” to “no match found” seems reasonable to me. If I understand correctly, that would resolve this issue for you?

I’ve updated the PR to emit “error: no match found in search range”.

Yes it does, thanks!

1 Like

The PR has landed. Thanks to everyone for taking a look.