-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[UTC] CHECK-EMPTY instead of skipping blank lines #165718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-testing-tools Author: Kunqiu Chen (Camsyn) ChangesPreviously, any blank lines in IR were ignored by UTC, leading to more fragile This change lets UTC, 1) emit Moreover, this change also aligns the behavior of IR check-gen to ASM check-gen, which has been emitting CHECK-EMPTY since a8a89c7. Full diff: https://github.com/llvm/llvm-project/pull/165718.diff 4 Files Affected:
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_empty.ll b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_empty.ll
new file mode 100644
index 0000000000000..abaa0ce2dd3b4
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_empty.ll
@@ -0,0 +1,29 @@
+; RUN: opt < %s -S | FileCheck %s
+
+; Test whether the UTC check empty lines instead of skipping them.
+define i32 @test(i32 %x) {
+entry:
+ br label %block1
+
+block1:
+ %cmp = icmp eq i32 %x, 0
+ br i1 %cmp, label %block2, label %exit1
+
+block2:
+ br i1 %cmp, label %block3, label %exit2
+
+block3:
+ br i1 %cmp, label %exit3, label %exit4
+
+exit1:
+ ret i32 0
+
+exit2:
+ ret i32 %x
+
+exit3:
+ ret i32 %x
+
+exit4:
+ ret i32 %x
+}
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_empty.ll.expected b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_empty.ll.expected
new file mode 100644
index 0000000000000..dc2a37907039e
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_empty.ll.expected
@@ -0,0 +1,57 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt < %s -S | FileCheck %s
+
+; Test whether the UTC check empty lines instead of skipping them.
+define i32 @test(i32 %x) {
+; CHECK-LABEL: define i32 @test(
+; CHECK-SAME: i32 [[X:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: br label %[[BLOCK1:.*]]
+; CHECK-EMPTY:
+; CHECK-NEXT: [[BLOCK1]]:
+; CHECK-NEXT: [[CMP:%.*]] = icmp eq i32 [[X]], 0
+; CHECK-NEXT: br i1 [[CMP]], label %[[BLOCK2:.*]], label %[[EXIT1:.*]]
+; CHECK-EMPTY:
+; CHECK-NEXT: [[BLOCK2]]:
+; CHECK-NEXT: br i1 [[CMP]], label %[[BLOCK3:.*]], label %[[EXIT2:.*]]
+; CHECK-EMPTY:
+; CHECK-NEXT: [[BLOCK3]]:
+; CHECK-NEXT: br i1 [[CMP]], label %[[EXIT3:.*]], label %[[EXIT4:.*]]
+; CHECK-EMPTY:
+; CHECK-NEXT: [[EXIT1]]:
+; CHECK-NEXT: ret i32 0
+; CHECK-EMPTY:
+; CHECK-NEXT: [[EXIT2]]:
+; CHECK-NEXT: ret i32 [[X]]
+; CHECK-EMPTY:
+; CHECK-NEXT: [[EXIT3]]:
+; CHECK-NEXT: ret i32 [[X]]
+; CHECK-EMPTY:
+; CHECK-NEXT: [[EXIT4]]:
+; CHECK-NEXT: ret i32 [[X]]
+;
+entry:
+ br label %block1
+
+block1:
+ %cmp = icmp eq i32 %x, 0
+ br i1 %cmp, label %block2, label %exit1
+
+block2:
+ br i1 %cmp, label %block3, label %exit2
+
+block3:
+ br i1 %cmp, label %exit3, label %exit4
+
+exit1:
+ ret i32 0
+
+exit2:
+ ret i32 %x
+
+exit3:
+ ret i32 %x
+
+exit4:
+ ret i32 %x
+}
diff --git a/llvm/test/tools/UpdateTestChecks/update_test_checks/check_empty.test b/llvm/test/tools/UpdateTestChecks/update_test_checks/check_empty.test
new file mode 100644
index 0000000000000..61e85152db951
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_test_checks/check_empty.test
@@ -0,0 +1,3 @@
+## test whether the UTC generates CHECK-EMPTY for blank lines
+# RUN: cp -f %S/Inputs/check_empty.ll %t.ll && %update_test_checks %t.ll
+# RUN: diff -u %t.ll %S/Inputs/check_empty.ll.expected
diff --git a/llvm/utils/UpdateTestChecks/common.py b/llvm/utils/UpdateTestChecks/common.py
index 8cd200c93a482..d9ef660ebdca8 100644
--- a/llvm/utils/UpdateTestChecks/common.py
+++ b/llvm/utils/UpdateTestChecks/common.py
@@ -2282,41 +2282,23 @@ def add_checks(
original_check_lines=original_check_lines.get(checkprefix),
)
- # This could be selectively enabled with an optional invocation argument.
- # Disabled for now: better to check everything. Be safe rather than sorry.
-
- # Handle the first line of the function body as a special case because
- # it's often just noise (a useless asm comment or entry label).
- # if func_body[0].startswith("#") or func_body[0].startswith("entry:"):
- # is_blank_line = True
- # else:
- # output_lines.append('%s %s: %s' % (comment_marker, checkprefix, func_body[0]))
- # is_blank_line = False
-
- is_blank_line = False
for func_line in func_body:
if func_line.strip() == "":
- is_blank_line = True
+ # Instead of skipping blank lines, using CHECK_EMPTY is more defensive.
+ output_lines.append(
+ "{} {}-EMPTY:".format(comment_marker, checkprefix)
+ )
continue
# Do not waste time checking IR comments.
func_line = SCRUB_IR_COMMENT_RE.sub(r"", func_line)
- # Skip blank lines instead of checking them.
- if is_blank_line:
- output_lines.append(
- "{} {}: {}".format(
- comment_marker, checkprefix, func_line
- )
- )
- else:
- check_suffix = "-NEXT" if not is_filtered else ""
- output_lines.append(
- "{} {}{}: {}".format(
- comment_marker, checkprefix, check_suffix, func_line
- )
+ check_suffix = "-NEXT" if not is_filtered else ""
+ output_lines.append(
+ "{} {}{}: {}".format(
+ comment_marker, checkprefix, check_suffix, func_line
)
- is_blank_line = False
+ )
# Add space between different check prefixes and also before the first
# line of code in the test function.
|
|
Should this change be gated by a new version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could potentially cause a lot of test churn when regenerating, so probably worth making it a new version. But I'll defer that decision to @nikic
|
Yes, this definitely needs to be versioned, as it's going to cause a massive amount of churn. You can add it to version 7 instead of creation a new one, as it's not the DEFAULT_VERSION yet. |
1748e4c to
2804b2c
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
| blank_line_indices = { | ||
| i for i, line in enumerate(func_body) if line.strip() == "" | ||
| } if ginfo.get_version() >= 7 else set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| blank_line_indices = { | |
| i for i, line in enumerate(func_body) if line.strip() == "" | |
| } if ginfo.get_version() >= 7 else set() | |
| blank_line_indices = set() | |
| if ginfo.get_version() >= 7: | |
| blank_line_indices = {i for i, line in enumerate(func_body) if line.strip() == ""} |
code formatter is not happy with the current one, maybe this here works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
race condition :)
| @@ -0,0 +1,29 @@ | |||
| ; RUN: opt < %s -S | FileCheck %s | |||
|
|
|||
| ; Test whether the UTC check empty lines instead of skipping them. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ; Test whether the UTC check empty lines instead of skipping them. | |
| ; Test whether UTC checks empty lines instead of skipping them. |
Previously, any blank lines in IR were ignored by UTC, leading to more fragile `CHECK`s being generated. This change lets UTC, 1) emit `CHECK-EMPTY` to check blank lines, and 2) generate more `CHECK-NEXT`s, landing the discussion llvm#165419 (comment). Moreover, this change also aligns the behavior of IR check-gen to ASM check-gen, which has been emitting `CHECK-EMPTY` since llvm@a8a89c7.
Previously, any blank lines in IR were ignored by UTC, leading to more fragile
CHECKs being generated.This change lets UTC, 1) emit
CHECK-EMPTYto check blank lines, and 2) generate moreCHECK-NEXTs, landing the discussion #165419 (comment).Moreover, this change also aligns the behavior of IR check-gen to ASM check-gen, which has been emitting
CHECK-EMPTYsince a8a89c7.