[AMDGPU][test] fix the error case in update_mc_test_check script #112731

broxigarchen · 2024-10-17T15:36:17Z

update_mc_test_check script handle the "error case testline" wrong in three cases:

when user select "--llvm-mc-binary" with a path, the script does not add "not" on top of the "--llvm-mc-binary" and thus getting non-zero exit code and failed.
When "not" is presented in runline while not all testlines are expected to fail, the script need to check if the "not" is needed when it execute llvm-mc line by line. Otherwise the script will fail on testline which is passing.
When there are multiple runlines, the error checkline need to use correct line offset for "[[LINE-X]]"

This patch solve these three issues

llvmbot · 2024-10-17T15:36:54Z

@llvm/pr-subscribers-testing-tools

@llvm/pr-subscribers-backend-amdgpu

Author: Brox Chen (broxigarchen)

Changes

update_mc_test_check script handle the error test( runline with "not") wrong in two cases:

when user select "--llvm-mc-binary" with a path, the script does not add "not" on top of the "--llvm-mc-binary" and thus getting non-zero exit code and failed.
When there are multiple testlines in the file while only not all testlines are expected to fail, the script need to check if the "not" is needed or not when it execute llvm-mc line by line. Otherwise the script will fail on testline which is passing.

This patch solve these two issues

Full diff: https://github.com/llvm/llvm-project/pull/112731.diff

3 Files Affected:

(modified) llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s (+2)
(modified) llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s.expected (+3)
(modified) llvm/utils/update_mc_test_checks.py (+26-6)

diff --git a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s
index 489bd1801d864a..b8a21f9e0162a1 100644
--- a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s
+++ b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s
@@ -1,3 +1,5 @@
 // RUN: not llvm-mc -triple=amdgcn -show-encoding %s 2>&1 | FileCheck --check-prefixes=CHECK %s
 
 v_bfrev_b32 v5, v299
+
+v_bfrev_b32 v5, v1
diff --git a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s.expected b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s.expected
index 0a0ad51d15e056..8cb098477ae3ae 100644
--- a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s.expected
+++ b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_err.s.expected
@@ -3,3 +3,6 @@
 
 v_bfrev_b32 v5, v299
 // CHECK: :[[@LINE-1]]:17: error: register index is out of range
+
+v_bfrev_b32 v5, v1
+// CHECK: v_bfrev_b32_e32 v5, v1                  ; encoding: [0x01,0x71,0x0a,0x7e]
diff --git a/llvm/utils/update_mc_test_checks.py b/llvm/utils/update_mc_test_checks.py
index f9f8cfdea418d0..cf6f8879daffcf 100755
--- a/llvm/utils/update_mc_test_checks.py
+++ b/llvm/utils/update_mc_test_checks.py
@@ -15,7 +15,6 @@
 
 mc_LIKE_TOOLS = [
     "llvm-mc",
-    "not llvm-mc",
 ]
 ERROR_RE = re.compile(r":\d+: (warning|error): .*")
 ERROR_CHECK_RE = re.compile(r"# COM: .*")
@@ -23,7 +22,7 @@
 COMMENT = {"asm": "//", "dasm": "#"}
 
 
-def invoke_tool(exe, cmd_args, testline, verbose=False):
+def invoke_tool(exe, prefix_not, cmd_args, testline, verbose=False):
     if isinstance(cmd_args, list):
         args = [applySubstitutions(a, substitutions) for a in cmd_args]
     else:
@@ -32,7 +31,20 @@ def invoke_tool(exe, cmd_args, testline, verbose=False):
     cmd = 'echo "' + testline + '" | ' + exe + " " + args
     if verbose:
         print("Command: ", cmd)
-    out = subprocess.check_output(cmd, shell=True)
+
+    # if not is used in runline, the command might or might not
+    # return non-zero code on a single line run
+    try:
+        out = subprocess.check_output(cmd, shell=True)
+    except:
+        if prefix_not:
+            cmd = 'echo "' + testline + '" | ' + "not " + exe + " " + args
+            if verbose:
+                print("Command: ", cmd)
+            out = subprocess.check_output(cmd, shell=True)
+        else:
+            raise Exception("Command '{}' return non-zero value".format(cmd))
+
     # Fix line endings to unix CR style.
     return out.decode().replace("\r\n", "\n")
 
@@ -151,11 +163,16 @@ def main():
             assert len(commands) >= 2
             mc_cmd = " | ".join(commands[:-1])
             filecheck_cmd = commands[-1]
-            mc_tool = mc_cmd.split(" ")[0]
 
             # special handling for negating exit status
-            if mc_tool == "not":
-                mc_tool = mc_tool + " " + mc_cmd.split(" ")[1]
+            prefix_not = ""
+            mc_cmd_args = mc_cmd.split(" ")
+            if mc_cmd_args[0] == "not":
+                prefix_not = mc_cmd_args[0]
+                mc_tool = mc_cmd_args[1]
+                mc_cmd = mc_cmd[len(mc_cmd_args[0]) :].strip()
+            else:
+                mc_tool = mc_cmd_args[0]
 
             triple_in_cmd = None
             m = common.TRIPLE_ARG_RE.search(mc_cmd)
@@ -188,6 +205,7 @@ def main():
                 (
                     check_prefixes,
                     mc_tool,
+                    prefix_not,
                     mc_cmd_args,
                     triple_in_cmd,
                     march_in_cmd,
@@ -204,6 +222,7 @@ def main():
         for (
             prefixes,
             mc_tool,
+            prefix_not,
             mc_args,
             triple_in_cmd,
             march_in_cmd,
@@ -222,6 +241,7 @@ def main():
                 # get output for each testline
                 out = invoke_tool(
                     ti.args.llvm_mc_binary or mc_tool,
+                    prefix_not,
                     mc_args,
                     line,
                     verbose=ti.args.verbose,

broxigarchen · 2024-10-17T20:48:49Z

The test failed seems unrelated to this patch

kosarev

Needs a test using the --llvm-mc-binary option?

llvm/utils/update_mc_test_checks.py

kosarev · 2024-10-18T10:36:20Z

llvm/utils/update_mc_test_checks.py

+            cmd = 'echo "' + testline + '" | ' + "not " + exe + " " + args
+            if verbose:
+                print("Command: ", cmd)
+            out = subprocess.check_output(cmd, shell=True, stderr=subprocess.DEVNULL)


This invokes the same thing again with not, but we already have the output from the first run? The docs reads:

https://docs.python.org/3/library/subprocess.html#subprocess.check_output

If the return code was non-zero it raises a CalledProcessError. The CalledProcessError object will have the return code in the returncode attribute and any output in the output attribute.

Let me try this. If we can reuse the output from the non-zero run, then yeah I think we can save one call here

Looks like we can not reuse the out from a try...except context. However, we might make it easier with another call run

llvm/utils/update_mc_test_checks.py

kosarev · 2024-10-18T10:38:49Z

llvm/utils/update_mc_test_checks.py

+        + " "
+        + prefix
+        + ": "
+        + ":[[@LINE-{}]]".format(line_offset)


Aren't we allowed to use f-strings in these scripts? I see update_any_test_checks.py does use them:

f"{utc_name}: not found (used in {testname})",

I think surely we can use f string. It seems the trend is to switch to f string? I will keep that in mind.

I agree an f-string would make this easier to read.

llvm/utils/update_mc_test_checks.py

broxigarchen · 2024-10-18T16:35:04Z

Needs a test using the --llvm-mc-binary option?

I am not sure if we can do this in the test here? since it requires a mc-binary location

broxigarchen · 2024-10-21T18:26:08Z

Pull from main and retrigger CI

kosarev · 2024-10-24T12:45:40Z

Needs a test using the --llvm-mc-binary option?

I am not sure if we can do this in the test here? since it requires a mc-binary location

When I use --llvm-mc-binary, which is my default way of using it, the script changes the NOTE: Assertions have been autogenerated by ... line to mention the --llvm-mc-binary option. This doesn't happen when I update llc tests.

broxigarchen · 2024-10-28T21:59:22Z

Needs a test using the --llvm-mc-binary option?

I am not sure if we can do this in the test here? since it requires a mc-binary location

When I use --llvm-mc-binary, which is my default way of using it, the script changes the NOTE: Assertions have been autogenerated by ... line to mention the --llvm-mc-binary option. This doesn't happen when I update llc tests.

updated the --llvm-mc-binary in common.py

arichardson

Looks sensible to me.

kosarev

LGTM.

llvm/utils/update_mc_test_checks.py

properly

broxigarchen · 2024-10-29T17:58:35Z

pull and resolve conflicts, waiting for CI

llvmbot added backend:AMDGPU testing-tools labels Oct 17, 2024

broxigarchen requested review from arichardson and kosarev October 17, 2024 15:36

broxigarchen force-pushed the update-mc-check-script-fix-not branch from a1971ab to 417bb06 Compare October 17, 2024 16:58

broxigarchen changed the title ~~[AMDGPU][test] fix the "not" issue in update_mc_test_check script~~ [AMDGPU][test] fix the error case in update_mc_test_check script Oct 17, 2024

broxigarchen force-pushed the update-mc-check-script-fix-not branch 2 times, most recently from 5bfadb6 to 48c97e9 Compare October 17, 2024 17:35

broxigarchen requested a review from Sisyph October 17, 2024 18:30

kosarev reviewed Oct 18, 2024

View reviewed changes

broxigarchen requested a review from kosarev October 21, 2024 15:32

broxigarchen force-pushed the update-mc-check-script-fix-not branch from e957db8 to e26ab37 Compare October 21, 2024 18:25

broxigarchen force-pushed the update-mc-check-script-fix-not branch from 5de06e2 to cc376f0 Compare October 28, 2024 21:59

arichardson reviewed Oct 28, 2024

View reviewed changes

kosarev approved these changes Oct 29, 2024

View reviewed changes

llvm/utils/update_mc_test_checks.py Outdated Show resolved Hide resolved

fix an issue in update_mc_script that the error case is not handle

9ced142

properly

broxigarchen force-pushed the update-mc-check-script-fix-not branch from 29fbfcc to 2cd440c Compare October 29, 2024 17:58

use check_rc

6a3905f

broxigarchen force-pushed the update-mc-check-script-fix-not branch from 2cd440c to 6a3905f Compare October 29, 2024 21:26

broxigarchen merged commit 6f973fd into llvm:main Oct 30, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMDGPU][test] fix the error case in update_mc_test_check script #112731

[AMDGPU][test] fix the error case in update_mc_test_check script #112731

broxigarchen commented Oct 17, 2024 •

edited

Loading

llvmbot commented Oct 17, 2024 •

edited

Loading

broxigarchen commented Oct 17, 2024

kosarev left a comment

kosarev Oct 18, 2024

broxigarchen Oct 18, 2024

broxigarchen Oct 18, 2024

kosarev Oct 18, 2024

broxigarchen Oct 18, 2024 •

edited

Loading

arichardson Oct 28, 2024

broxigarchen commented Oct 18, 2024

broxigarchen commented Oct 21, 2024

kosarev commented Oct 24, 2024

broxigarchen commented Oct 28, 2024

arichardson left a comment

kosarev left a comment

broxigarchen commented Oct 29, 2024

[AMDGPU][test] fix the error case in update_mc_test_check script #112731

[AMDGPU][test] fix the error case in update_mc_test_check script #112731

Conversation

broxigarchen commented Oct 17, 2024 • edited Loading

llvmbot commented Oct 17, 2024 • edited Loading

broxigarchen commented Oct 17, 2024

kosarev left a comment

Choose a reason for hiding this comment

kosarev Oct 18, 2024

Choose a reason for hiding this comment

broxigarchen Oct 18, 2024

Choose a reason for hiding this comment

broxigarchen Oct 18, 2024

Choose a reason for hiding this comment

kosarev Oct 18, 2024

Choose a reason for hiding this comment

broxigarchen Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

arichardson Oct 28, 2024

Choose a reason for hiding this comment

broxigarchen commented Oct 18, 2024

broxigarchen commented Oct 21, 2024

kosarev commented Oct 24, 2024

broxigarchen commented Oct 28, 2024

arichardson left a comment

Choose a reason for hiding this comment

kosarev left a comment

Choose a reason for hiding this comment

broxigarchen commented Oct 29, 2024

broxigarchen commented Oct 17, 2024 •

edited

Loading

llvmbot commented Oct 17, 2024 •

edited

Loading

broxigarchen Oct 18, 2024 •

edited

Loading