Skip to content
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

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

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

broxigarchen
Copy link
Contributor

@broxigarchen broxigarchen commented Oct 17, 2024

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

  1. 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.
  2. 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.
  3. When there are multiple runlines, the error checkline need to use correct line offset for "[[LINE-X]]"

This patch solve these three issues

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 17, 2024

@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:

  1. 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.
  2. 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 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 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
Copy link
Contributor Author

The test failed seems unrelated to this patch

Copy link
Collaborator

@kosarev kosarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

llvm/utils/update_mc_test_checks.py Show resolved Hide resolved
cmd = 'echo "' + testline + '" | ' + "not " + exe + " " + args
if verbose:
print("Command: ", cmd)
out = subprocess.check_output(cmd, shell=True, stderr=subprocess.DEVNULL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
+ " "
+ prefix
+ ": "
+ ":[[@LINE-{}]]".format(line_offset)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})",

Copy link
Contributor Author

@broxigarchen broxigarchen Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

llvm/utils/update_mc_test_checks.py Outdated Show resolved Hide resolved
@broxigarchen
Copy link
Contributor Author

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
Copy link
Contributor Author

Pull from main and retrigger CI

@kosarev
Copy link
Collaborator

kosarev commented Oct 24, 2024

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
Copy link
Contributor Author

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

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible to me.

Copy link
Collaborator

@kosarev kosarev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

llvm/utils/update_mc_test_checks.py Outdated Show resolved Hide resolved
@broxigarchen
Copy link
Contributor Author

pull and resolve conflicts, waiting for CI

@broxigarchen broxigarchen merged commit 6f973fd into llvm:main Oct 30, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants