Normalize filecheck directives #128018

tgross35 · 2024-07-20T21:34:59Z

Typically in the LLVM repo, filecheck prefixes will be uppercase (this is always true) and start with CHECK- (true a bit more than half the time). Currently we using exact revision names as filecheck directives, in the same case, so they don't really look like filecheck directives in code:

    // CHECK-LABEL: foo
    // x86: pop    rbp
    // aarch64: add     sp, sp, #32
    // CHECK-NEXT ret

Some tests use uppercase revisions, which I suspect this is likely to make them look more like typical filecheck.

So, do the following:

In compiletest, change the check prefixes to always be CHECK-{revision.to_uppercase()} rather than just revision
For any tests that currently have uppercase revision names, change them to lowercase
Update all revision directives to uppercase and start with CHECK-
For anything that used an invalid prefix as a way to skip a check (e.g. FIXME-CHECK:), change those to use COM: so we can eventually check for invalid check prefixes.

The result looks more consistent and makes it easier to identify and grep for directives. As a bonus, some editors make them stand out with highlighting (which currently happens for most directives, just not those from revisions):

I used the following to verify there aren't any directives that missed an update:

rg --pcre2 '^\s*//\s*(?!(CHECK|COM))\S*:(?!(//|:))' \
    tests/codegen tests/assembly/ tests/mir-opt/

And this to identify uppercase revisions:

rg '//@\s*revisions:.*[A-Z]'

Marking as a draft since I did the changes, but still need to go through by hand and verify there is nothing that looks out of place.

This should get a .git-blame-ignore-revs entry.

r? @jieyouxu

Typically, filecheck prefixes will be uppercase (always true) and start with `CHECK-` (almost always true). Currently we allow using revision names as filecheck directives, but they are passed directly. That means that our directives are exactly what the revision name is, in the same case, so they only look like filecheck directives if the revision name is uppercase (usually they are lowercase). Update this so that we always uppercase revision names and prefix them with `CHECK-` when used as directives. This is better for consistency, makes it easier to identify directives in the tests, and has the nice side effect that some editors will make the directive stand out by highlighting it (which currently happens for most directives, just not those from revisions).

workingjubilee · 2024-07-20T21:37:28Z

obvious hazard: a revision named next

workingjubilee

I feel I want this general idea but I'm not sure exactly this way is right.

workingjubilee · 2024-07-20T21:51:41Z

tests/codegen/comparison-operators-2-tuple.rs

-    // FIXME-CHECK-DAG: %[[EQ:.+]] = icmp eq i16 %[[A0]], %[[B0]]
-    // FIXME-CHECK-DAG: %[[CMP0:.+]] = icmp sge i16 %[[A0]], %[[B0]]
-    // FIXME-CHECK-DAG: %[[CMP1:.+]] = icmp uge i16 %[[A1]], %[[B1]]
-    // FIXME-CHECK: %[[R:.+]] = select i1 %[[EQ]], i1 %[[CMP1]], i1 %[[CMP0]]
-    // FIXME-CHECK: ret i1 %[[R]]


silly option to instead make this valid: emit FIXME-CHECK as a directive but check that it's failing.

Ooh I like that idea. I guess we would have to do something like transform it to CHECK-NOT?

I guess that would cause problems if e.g. one line matches. Maybe there could be an implicit FIXME rev and then assert that it fails.

I am sort of working on a followup that will (hopefully) let us locate filecheck directives and make sure we don't have anything unexpected, that would probably be easier to do with that available.

right, I would assume it would be an implicit rev and run as a separate test.

workingjubilee · 2024-07-20T21:52:51Z

tests/assembly/asm/x86-types.rs

+// CHECK-X86_64: mov r{{[a-z0-9]+}}, r{{[a-z0-9]+}}
+// CHECK-I686: mov e{{[a-z0-9]+}}, e{{[a-z0-9]+}}


Hmm. I feel like I would prefer CHECK-x86_64 and CHECK-i686? i.e. exact match of rev name.

Uppercasing was some of the point of this since it matches builtin directives, and is what LLVM uses (209 completely lowercase vs. 24.4k completely uppercase, mixed < 1k). So I think it is some of what people are used to, which I think is why we seem to have a mix of uppercase and lowercase revision names.

(plus, at least in my editor (helix), it only highlights if the string leading up to the : is all uppercase, so it looks more consistent. Obviously that is the about the furthest thing from a good reason to do anything, and I have no idea about other editors, but bonuses are nice).

Sure. I can get used to it, probably, the other concern about ordering of the rev and CHECK would rate higher if any.

I like X86_64 or CHECK-x86_64. When I encounter unfamiliar terms, all-uppercase is difficult to understand.

Agreed with the revision name exact matching. I'm not sure if in compiletest we currently treat revision names as case-insensitive (if we don't, then we probably should), but IMO an exact case match against a revision name is less confusing. For target names especially, sometimes the casing contributes to more familiarity.

I don't have a strong opinion on this stylistically, anyway.

I like X86_64 or CHECK-x86_64. When I encounter unfamiliar terms, all-uppercase is difficult to understand.

I would counter that it is more likely for someone newish to understand that CHECK-X86_64: pertains to x86_64, than it is for someone newish to understand that X86_64: is indicating a pattern that gets checked.

workingjubilee · 2024-07-20T21:54:18Z

tests/codegen/simd/packed-simd.rs

@@ -51,7 +51,7 @@ pub fn square_packed_full(x: Simd<f32, 3>) -> FullSimd<f32, 3> {
 pub fn square_packed(x: Simd<f32, 3>) -> Simd<f32, 3> {
    // CHECK-NEXT: start
    // CHECK-NEXT: load <3 x float>
-    // noopt-NEXT: load <3 x float>
+    // CHECK-NOOPT-NEXT: load <3 x float>


This was made slightly harder to read. The rev name was sorta intentional, to make it stand out more, but now it blurs into the other CHECK*NEXTs and becomes easier to overlook. maybe it should be "{rev}-CHECK"?

hm, I wonder if FileCheck would accept [{REV}]-CHECK?

hm, I wonder if FileCheck would accept [{REV}]-CHECK?

Any string similar to a variable name is ok.

Since revisions are now always passed as `CHECK-{rev.to_uppercase()}`, update test cases to use this format. There were also some test files that used uppercase names, presumably so the directives would match filecheck. Normalize these revision names to lowercase. Lastly, remove use of invalid labels (such as `FIXME-CHECK:`) to skip filecheck directives. (`COM: CHECK ...` is the syntax for a comment). The below query can be used to ensure that there are no missing directives: rg --pcre2 '^\s*//\s*(?!(CHECK|COM))\S*:(?!(//|:))' \ tests/codegen tests/assembly/ tests/mir-opt/ (Note that the above may report some non-filecheck FIXMEs and other comments).

jieyouxu · 2024-07-21T06:22:41Z

To me normalizing filecheck directives like this makes sense, but I'm not the most familiar with filecheck, so I'll request a sanity check + vibe check early from people who do:

cc @scottmcm since I know you are quite familiar with filecheck and codegen tests, does normalizing filecheck directives like this make the test reading/writing experience better/worse?

@rustbot ping llvm
Dear wg-llvm members, does normalizing llvm filecheck directives in our tests seem like a good idea? From what I can tell, here we are trying to:

Use a more consistent filecheck prefix (make sure CHECK is present).
Use a more consistent casing (always upper case).

rustbot · 2024-07-21T06:22:44Z

Hey LLVM ICE-breakers! This bug has been identified as a good
"LLVM ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @ayazhafiz @camelid @comex @cuviper @DianQK @hdhoang @henryboisdequin @heyrutvik @higuoxing @JOE1994 @jryans @luqmana @mmilenko @nagisa @nikic @Noah-Kennedy @SiavoshZarrasvand @vertexclique

jieyouxu · 2024-07-21T06:26:44Z

Err I'm so sorry, I think that's not the intended llvm group I wanted to ping... maybe cc @rust-lang/wg-llvm? But if anyone pinged has any filecheck experience and has feedback for our test suites' llvm filecheck syntax changes here, that is also very welcomed.

tgross35 · 2024-07-21T06:27:37Z

^ wow does it just ping everybody related to LLVM? More people than I would have expected lol. That definitely seems like an unfortunately large ping group to have named llvm (opened a Zulip topic to discuss changing it).

In any case good call, thanks for the follow up.

DianQK

I think we use uppercase to enhance readability here.

DianQK · 2024-07-21T08:23:32Z

src/tools/compiletest/src/runtest.rs

        if let Some(rev) = self.revision {
-            filecheck.arg("--check-prefix").arg(rev);
+            filecheck.arg(format!("--check-prefix=CHECK-{}", rev.to_uppercase()));


I'm not sure if it's appropriate to add CHECK- to all prefixes. For me, uppercase is enough to emphasize that this is not an ordinary comment.

Hmm, I don't mind using CHECK- for everything either, but since it's a hidden convention, we at least need documentation to explain it.

I did update the in-repo docs as part of this https://github.com/rust-lang/rust/pull/128018/files#diff-b40381314bcd0d5dd401af44ac13150c8129d97c97c6a76cbae9e3841a551329 :) and I would follow this with a dev guide update

DianQK · 2024-07-21T08:26:02Z

tests/assembly/asm/x86-types.rs

+// CHECK-X86_64: mov r{{[a-z0-9]+}}, r{{[a-z0-9]+}}
+// CHECK-I686: mov e{{[a-z0-9]+}}, e{{[a-z0-9]+}}


I like X86_64 or CHECK-x86_64. When I encounter unfamiliar terms, all-uppercase is difficult to understand.

DianQK · 2024-07-21T08:33:47Z

tests/codegen/sanitizer/sanitizer-recover.rs

@@ -3,44 +3,44 @@
 //
 //@ needs-sanitizer-address
 //@ needs-sanitizer-memory
-//@ revisions:ASAN ASAN-RECOVER MSAN MSAN-RECOVER MSAN-RECOVER-LTO
+//@ revisions: asan asan-recover msan msan-recover msan-recover-lto


Maybe this should be a separate PR?

Good point; if this goes forward, I will split the upper->lowercase items out.

DianQK · 2024-07-21T08:34:50Z

tests/codegen/sanitizer/sanitizer-recover.rs

+// CHECK-MSAN-RECOVER-NOT:     unreachable
+// CHECK-MSAN-RECOVER:       }
+//
+// CHECK-MSAN-RECOVER-LTO-LABEL: define dso_local noundef i32 @penguin(


A long check mark, I think, CHECK- reduces overall readability. :)

DianQK · 2024-07-21T08:35:57Z

tests/codegen/simd/packed-simd.rs

@@ -51,7 +51,7 @@ pub fn square_packed_full(x: Simd<f32, 3>) -> FullSimd<f32, 3> {
 pub fn square_packed(x: Simd<f32, 3>) -> Simd<f32, 3> {
    // CHECK-NEXT: start
    // CHECK-NEXT: load <3 x float>
-    // noopt-NEXT: load <3 x float>
+    // CHECK-NOOPT-NEXT: load <3 x float>


hm, I wonder if FileCheck would accept [{REV}]-CHECK?

Any string similar to a variable name is ok.

nikic · 2024-07-21T09:02:45Z

Don't really have a strong opinion on how we write this. I don't think this part of the PR description is correct though:

Typically in the LLVM repo, filecheck prefixes will be uppercase (this is always true) and start with CHECK- (true a bit more than half the time).

While the first part is true, LLVM does not have a convention to prefix everything with CHECK-. It's common to use CHECK as a common fallback prefix for --check-prefixes, but the non-fallback prefixes generally do not start with CHECK-, especially if the check prefixes are used to distinguish subtargets. You'll see lots of --check-prefixes=CHECK,AVX2 and very little --check-prefixes=CHECK,CHECK-AVX2.

Always using CHECK- may make sense for Rust though, which has a lot more people not familiar with FileCheck working on it. If it helps people distinguish what is and isn't a FileCheck directive, I don't mind.

jieyouxu · 2024-07-21T09:14:17Z

Always using CHECK- may make sense for Rust though, which has a lot more people not familiar with FileCheck working on it. If it helps people distinguish what is and isn't a FileCheck directive, I don't mind.

Ever since we migrated from // <compiletest_directive> to //@ <compiletest_directive>, at least it's easier to tell which are compiletest directives and which are FileCheck directives. I don't have a strong opinion on whether we want the CHECK- prefix either.

tgross35 · 2024-07-21T18:58:07Z

Don't really have a strong opinion on how we write this. I don't think this part of the PR description is correct though:

Typically in the LLVM repo, filecheck prefixes will be uppercase (this is always true) and start with CHECK- (true a bit more than half the time).

While the first part is true, LLVM does not have a convention to prefix everything with CHECK-. It's common to use CHECK as a common fallback prefix for --check-prefixes, but the non-fallback prefixes generally do not start with CHECK-, especially if the check prefixes are used to distinguish subtargets. You'll see lots of --check-prefixes=CHECK,AVX2 and very little --check-prefixes=CHECK,CHECK-AVX2.

I was just going off a rough search - 7.6k out of 15.7k total files seem to use prefixes starting with CHECK. I guess it is slightly less than half, not slightly more.

Always using CHECK- may make sense for Rust though, which has a lot more people not familiar with FileCheck working on it. If it helps people distinguish what is and isn't a FileCheck directive, I don't mind.

I think it is more obvious that CHECK-X86: is just a variant of plain CHECK, which helps if you are not familiar with the format. And it gives something easier to search for (compared to just searching "X86:").

I feel most strongly about uppercasing since there is strong precedent in LLVM, it reads better with -NOT -NEXT etc suffixes, and I don't like the status quo of having a portion of tests using uppercase revisions so their filecheck directives look more like LLVM.

CHECK- I feel slightly less strongly about. I still think this is helpful for newcomers to the format, and appreciate that it looks much more consistent. Also nice for tooling; a consistent prefix makes it easier for us to flag accidentally unused directives, or find things that look like they are supposed to be a directive but aren't getting picked up, and eventually drop --allow-unused-prefixes (I am working on these). I don't think there is any strong argument against it as long as this is documented well.

(There are a couple comments here about certain CHECK-REVNAMEs becoming less readable, but I think that is easier fixed by changing a few revision names than trying to build a global format around them. I am also not opposed to the proposed REV-CHECK, even though it is less common in LLVM I think it achieves similar goals).

In any case, I can change as needed. Suppose we may as well wait for Scott and any others to chime in.

workingjubilee · 2024-07-21T21:45:15Z

Despite the "slightly" qualifier, I do feel more strongly about the {REV}-CHECK ordering.

When I scan code, I am often looking for breaks in patterns, and it is easiest to do that if the initial prefix for different revs is visually distinct. Of course, a revision named czech (resulting in a CZECH-NEXT) would also be bad, even if it was CZECH-CHECK-NEXT, simply because it has enough of the same letters. But I don't expect many revisions named for the Czech Republic, despite it producing apparently a significant amount of silicon.

bors · 2024-07-30T09:04:54Z

☔ The latest upstream changes (presumably #128378) made this pull request unmergeable. Please resolve the merge conflicts.

jieyouxu · 2024-08-16T09:05:20Z

@tgross35 since there's some different preferences and I want to gauge consensus and preferences for the people who will be maintaining/reviewing/reading the tests, I'll draft a T-compiler MCP later tonight or maybe tomorrow, so people can register concerns if they have strong preferences and suggestions. By filing a T-compiler MCP, we can make sure at least T-compiler reviewers are aware of the normalized FileCheck syntax (I'll help with follow-up doc updates and such). I'll also tag wg-llvm members and Jubilee (and Scott) along since Jubilee has some preferences and Scott reads/writes/reviews quite a bit of FileCheck tests as well. I'll try to come up with a draft MCP and then share it with you in case you want to propose any improvements/changes.

(Also sorry for the review delay, I ignored the draft PRs for a while due to other non-draft PRs)

rustbot assigned jieyouxu Jul 20, 2024

This comment has been minimized.

Sign in to view

workingjubilee reviewed Jul 20, 2024

View reviewed changes

tgross35 force-pushed the normalize-filecheck-directives branch 3 times, most recently from fb33fb8 to 6b8b6b7 Compare July 20, 2024 23:20

tgross35 force-pushed the normalize-filecheck-directives branch from 6b8b6b7 to 144fa44 Compare July 20, 2024 23:23

rustbot added the ICEBreaker-LLVM Bugs identified for the LLVM ICE-breaker group label Jul 21, 2024

jieyouxu removed the ICEBreaker-LLVM Bugs identified for the LLVM ICE-breaker group label Jul 21, 2024

DianQK reviewed Jul 21, 2024

View reviewed changes

tgross35 mentioned this pull request Jul 22, 2024

Rewrite the compiletest directive parser #128070

Draft

jieyouxu mentioned this pull request Aug 17, 2024

tests: fix filecheck typos #125626

Open

		// CHECK-X86_64: mov r{{[a-z0-9]+}}, r{{[a-z0-9]+}}
		// CHECK-I686: mov e{{[a-z0-9]+}}, e{{[a-z0-9]+}}

Normalize filecheck directives #128018

Are you sure you want to change the base?

Normalize filecheck directives #128018

Conversation

tgross35 commented Jul 20, 2024 • edited Loading

workingjubilee commented Jul 20, 2024

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross35 Jul 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Jul 21, 2024

rustbot commented Jul 21, 2024

jieyouxu commented Jul 21, 2024 • edited Loading

tgross35 commented Jul 21, 2024 • edited Loading

DianQK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Jul 21, 2024

jieyouxu commented Jul 21, 2024

tgross35 commented Jul 21, 2024 • edited Loading

workingjubilee commented Jul 21, 2024

bors commented Jul 30, 2024

jieyouxu commented Aug 16, 2024 • edited Loading

tgross35 commented Jul 20, 2024 •

edited

Loading

tgross35 Jul 21, 2024 •

edited

Loading

jieyouxu commented Jul 21, 2024 •

edited

Loading

tgross35 commented Jul 21, 2024 •

edited

Loading

tgross35 commented Jul 21, 2024 •

edited

Loading

jieyouxu commented Aug 16, 2024 •

edited

Loading