[AMDGPU][test]added unique and sort options for update_mc_test_check script #111769

broxigarchen · 2024-10-09T22:14:55Z

add a unique and a sort option to the update_mc_test_check script.

These mc asm/dasm files are usually large in number of lines, and these lines are mostly similar to each other. These options can be useful when maintainer is merging or resolving conflicts by making the file identifical

Also fixed a small issue in asm/dasm such that the auto generated header line is

asm using ";" instead of "//" as comment marker
dasm using ";" instead of "#" as comment marker

llvmbot · 2024-10-10T05:29:46Z

@llvm/pr-subscribers-testing-tools

@llvm/pr-subscribers-backend-amdgpu

Author: Brox Chen (broxigarchen)

Changes

add a remove duplication option to the update_mc_test_check script.

These mc asm/dasm files are usually large in number of lines. This option can be useful when maintainer is merging or resolving conflicts when they are multiple duplications

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

6 Files Affected:

(added) llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s (+5)
(added) llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s.expected (+6)
(added) llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt (+5)
(added) llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt.expected (+6)
(added) llvm/test/tools/UpdateTestChecks/update_mc_test_checks/amdgpu-remove-duplicates.test (+7)
(modified) llvm/utils/update_mc_test_checks.py (+21-6)

diff --git a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s
new file mode 100644
index 00000000000000..49f02cb5cb89c7
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s
@@ -0,0 +1,5 @@
+// RUN: llvm-mc -triple=amdgcn -show-encoding %s 2>&1 | FileCheck --check-prefixes=CHECK %s
+
+v_bfrev_b32 v5, v1
+
+v_bfrev_b32 v5, v1
diff --git a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s.expected b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s.expected
new file mode 100644
index 00000000000000..368143dd40af5b
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_asm_remove_duplicates.s.expected
@@ -0,0 +1,6 @@
+; NOTE: Assertions have been autogenerated by utils/update_mc_test_checks.py
+// RUN: llvm-mc -triple=amdgcn -show-encoding %s 2>&1 | FileCheck --check-prefixes=CHECK %s
+
+v_bfrev_b32 v5, v1
+// CHECK: v_bfrev_b32_e32 v5, v1                  ; encoding: [0x01,0x71,0x0a,0x7e]
+
diff --git a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt
new file mode 100644
index 00000000000000..3d0d49ddeea425
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt
@@ -0,0 +1,5 @@
+# RUN: llvm-mc -triple=amdgcn -mcpu=gfx1100 -disassemble -show-encoding %s 2>&1 | FileCheck -check-prefixes=CHECK %s
+
+0x00,0x00,0x00,0x7e
+
+0x00,0x00,0x00,0x7e
diff --git a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt.expected b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt.expected
new file mode 100644
index 00000000000000..c1b4a07328dac6
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_remove_duplicates.txt.expected
@@ -0,0 +1,6 @@
+; NOTE: Assertions have been autogenerated by utils/update_mc_test_checks.py
+# RUN: llvm-mc -triple=amdgcn -mcpu=gfx1100 -disassemble -show-encoding %s 2>&1 | FileCheck -check-prefixes=CHECK %s
+
+0x00,0x00,0x00,0x7e
+# CHECK: v_nop                                   ; encoding: [0x00,0x00,0x00,0x7e]
+#
diff --git a/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/amdgpu-remove-duplicates.test b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/amdgpu-remove-duplicates.test
new file mode 100644
index 00000000000000..df7d56a460f45e
--- /dev/null
+++ b/llvm/test/tools/UpdateTestChecks/update_mc_test_checks/amdgpu-remove-duplicates.test
@@ -0,0 +1,7 @@
+# REQUIRES: amdgpu-registered-target
+## Check that remove duplicate is working
+
+# RUN: cp -f %S/Inputs/amdgpu_asm_remove_duplicates.s %t.s && %update_mc_test_checks --remove-duplicate %t.s
+# RUN: diff -u %S/Inputs/amdgpu_asm_remove_duplicates.s.expected %t.s
+# RUN: cp -f %S/Inputs/amdgpu_dasm_remove_duplicates.txt %t.txt && %update_mc_test_checks --remove-duplicate %t.txt
+# RUN: diff -u %S/Inputs/amdgpu_dasm_remove_duplicates.txt.expected %t.txt
diff --git a/llvm/utils/update_mc_test_checks.py b/llvm/utils/update_mc_test_checks.py
index f9f8cfdea418d0..0e92fefaeeefd8 100755
--- a/llvm/utils/update_mc_test_checks.py
+++ b/llvm/utils/update_mc_test_checks.py
@@ -118,6 +118,12 @@ def main():
         default=None,
         help="Set a default -march for when neither triple nor arch are found in a RUN line",
     )
+
+    parser.add_argument(
+        "--remove-duplicate",
+        action=argparse.BooleanOptionalAction,
+        help="remove duplicated test line if found",
+    )
     parser.add_argument("tests", nargs="+")
     initial_args = common.parse_commandline_args(parser)
 
@@ -196,6 +202,10 @@ def main():
 
         # find all test line from input
         testlines = [l for l in ti.input_lines if isTestLine(l, mc_mode)]
+        # remove duplicated lines to save running time
+        testlines = list(dict.fromkeys(testlines))
+        common.debug("Valid test line found: ", len(testlines))
+
         run_list_size = len(run_list)
         testnum = len(testlines)
 
@@ -233,7 +243,7 @@ def main():
             raw_prefixes.append(prefixes)
 
         output_lines = []
-        generated_prefixes = []
+        generated_prefixes = {}
         used_prefixes = set()
         prefix_set = set([prefix for p in run_list for prefix in p[0]])
         common.debug("Rewriting FileCheck prefixes:", str(prefix_set))
@@ -298,16 +308,21 @@ def main():
                     else:
                         gen_prefix += getStdCheckLine(prefix, o, mc_mode)
 
-            generated_prefixes.append(gen_prefix.rstrip("\n"))
+            generated_prefixes[input_line] = gen_prefix.rstrip("\n")
 
         # write output
-        prefix_id = 0
+        written_lines = set()
         for input_info in ti.iterlines(output_lines):
             input_line = input_info.line
-            if isTestLine(input_line, mc_mode):
+            if input_line in testlines:
+                if ti.args.remove_duplicate:
+                    if input_line in written_lines:
+                        common.debug("Duplicated line skipped: ", input_line)
+                        continue
+                    else:
+                        written_lines.add(input_line)
                 output_lines.append(input_line)
-                output_lines.append(generated_prefixes[prefix_id])
-                prefix_id += 1
+                output_lines.append(generated_prefixes[input_line])
 
             elif should_add_line_to_output(input_line, prefix_set, mc_mode):
                 output_lines.append(input_line)

arichardson

Could you add a test for sorting with an input that contains some comments.

Ideally those comments would remaing attached to the line they were on before.

broxigarchen · 2024-10-10T23:57:40Z

Could you add a test for sorting with an input that contains some comments.

Ideally those comments would remaing attached to the line they were on before.

added a sort_with_comment test

The comment within the sameline or within the next continued line will work

TESTLINE  // comment

TESTLINE
// coment
// coment

For these cases it will not

 // comment
TESTLINE

TESTLINE

// coment

kosarev

What is your idea about sorting all-comments cases, like:

// FIXME: For atomic instructions, glc must be placed immediately following
// the data regiser.  These forms aren't currently supported:
// FIXME: offset:0 required
// flat_atomic_add v1, v[3:4], v5 slc glc

?

llvm/utils/update_mc_test_checks.py

kosarev · 2024-10-11T10:17:57Z

llvm/utils/update_mc_test_checks.py

+    parser.add_argument(
+        "--sort",
+        action="store_true",
+        default=False,


I guess not having this enabled by default might be problematic. Imagine some test cases added without sorting at random positions in one patch and then sorting them in another patch as some more test cases are added. This would also move cases added in the first patch. (I assume the idea is that ultimately we want keep all MC test files sorted at all times.)

The same reason as above. I think it's better asking user to run --sort manually.

I think we can add a hint by checking the auto-generated header line
# auto updated by update_mc_test_check.py UTC_ARGS: --sort

if there is a --sort there but there is no --sort being passed by the arguments, we can shot a warning. So that when anyone is trying to update a sorted file without passing --sort they will know it

llvm/utils/update_mc_test_checks.py

github-actions · 2024-10-17T00:01:09Z

✅ With the latest revision this PR passed the Python code formatter.

llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_sort.txt.expected

broxigarchen · 2024-10-18T14:59:14Z

Adding more reviewers to seek a wider consensus on sorting MC tests throughout

broxigarchen · 2024-10-19T18:38:14Z

Adding more reviewers to seek a wider consensus on sorting MC tests throughout

Discussed offline with Joe and Ivan. The dasm sorting might not be very useful right now when sorted with hex input, and we might not need it after dasm test are merged into asm test.

For this patch, drop the support of sorting for dasm test. Added a error message so that script error out if dasm test is passed with --sort

broxigarchen · 2024-10-24T19:16:05Z

ping!

) This is a non-functional change update GFX11/GFX12 VOPC/VOPCX asm/dasm test for true16/fake16: 1. duplicate files to be true16/fake16 by adding "-mattr=+real-true16/-mattr=-real-true16" while true16 test file will be updated to true16 format when the true16 instructions are supported 2. sort "*t16_err.s" and "*t16_promote.s" tests to alphabetic order. tests to alphabetic order. This is for the upcoming true16 mc changes, and mainly trying to help repo maintainer to resolve conflicts in the tests quickly. A script is proposed to help for the sorting #111769. Since these two files are t16 only, it should not create conflicts in downstream branches 3. add `-filetype=null` to seperate stdout and stderr to avoid disordered output from llvm-mc

Sisyph

LGTM, but please wait for @kosarev

Sisyph · 2024-10-28T19:11:32Z

Since this script is now being used for in tree tests (b27aceb) , I would recommend landing it now. If anything arises in post commit review, please re-run the script on the affected tests.

kosarev

LGTM with nits.

llvm/utils/update_mc_test_checks.py

kosarev · 2024-10-30T11:17:28Z

llvm/utils/update_mc_test_checks.py

@@ -6,6 +6,7 @@
 from __future__ import print_function

 import argparse
+import functools


This is unused now.

I'll remove this in another patch

…113101) This is a non-functional change update GFX11/GFX12 VOP2 asm/dasm test for true16/fake16: 1. duplicate files to be true16/fake16 by adding "-mattr=+real-true16/-mattr=-real-true16" while true16 test file will be updated to true16 format when the true16 instructions are supported 2. sort "*t16_err.s" and "*t16_promote.s" tests to alphabetic order. This is for the upcoming true16 mc changes, and mainly trying to help repo maintainer to resolve conflicts in the tests quickly. A script is proposed to help for the sorting #111769. Since these two files are t16 only, it should not create conflicts in downstream branches 3. add -filetype=null to seperate stdout and stderr to avoid disordered output from llvm-mc

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch 2 times, most recently from 1b938bf to ac8ee83 Compare October 9, 2024 22:20

broxigarchen changed the title ~~[AMDGPU][test]added remove dup options~~ [AMDGPU][test]added remove duplicate options Oct 9, 2024

broxigarchen changed the title ~~[AMDGPU][test]added remove duplicate options~~ [AMDGPU][test]added remove duplicate options for update_mc_test_check script Oct 9, 2024

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from ac8ee83 to 4756c55 Compare October 10, 2024 05:28

broxigarchen marked this pull request as ready for review October 10, 2024 05:29

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

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch 2 times, most recently from 9effb5a to 26f9d92 Compare October 10, 2024 21:55

broxigarchen requested review from arichardson and kosarev October 10, 2024 22:10

arichardson reviewed Oct 10, 2024

View reviewed changes

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from 26f9d92 to 98ece71 Compare October 10, 2024 23:54

broxigarchen requested a review from arichardson October 10, 2024 23:58

kosarev reviewed Oct 11, 2024

View reviewed changes

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

kosarev reviewed Oct 11, 2024

View reviewed changes

jayfoad reviewed Oct 16, 2024

View reviewed changes

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

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from 8a61029 to 0bad6cf Compare October 17, 2024 00:00

Sisyph reviewed Oct 17, 2024

View reviewed changes

llvm/test/tools/UpdateTestChecks/update_mc_test_checks/Inputs/amdgpu_dasm_sort.txt.expected Outdated Show resolved Hide resolved

broxigarchen requested review from kosarev and jayfoad October 17, 2024 14:57

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from 13febff to 107d161 Compare October 17, 2024 15:01

broxigarchen changed the title ~~[AMDGPU][test]added remove duplicate options for update_mc_test_check script~~ [AMDGPU][test]added unique and sort options for update_mc_test_check script Oct 17, 2024

broxigarchen mentioned this pull request Oct 18, 2024

[AMDGPU][True16][test] update VOPC/VOPCX test for true16/fake16 #112829

Merged

broxigarchen requested a review from nhaehnle October 18, 2024 14:59

broxigarchen requested review from rampitec, mariusz-sikora-at-amd and mbrkusanin October 18, 2024 14:59

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch 2 times, most recently from 7d5e484 to 0eb494c Compare October 19, 2024 18:35

broxigarchen mentioned this pull request Oct 20, 2024

[AMDGPU][True16][test] update VOP2 asm/dasm file with true16/fake16 #113101

Merged

Sisyph approved these changes 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

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

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

add a unique and sort option to update_mc_test script

788194d

broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from 0eb494c to 788194d Compare October 29, 2024 15:25

broxigarchen merged commit 528e975 into llvm:main Oct 29, 2024
8 checks passed

kosarev reviewed Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMDGPU][test]added unique and sort options for update_mc_test_check script #111769

[AMDGPU][test]added unique and sort options for update_mc_test_check script #111769

broxigarchen commented Oct 9, 2024 •

edited

Loading

llvmbot commented Oct 10, 2024 •

edited

Loading

arichardson left a comment

broxigarchen commented Oct 10, 2024 •

edited

Loading

kosarev left a comment

kosarev Oct 11, 2024

broxigarchen Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

broxigarchen commented Oct 18, 2024

broxigarchen commented Oct 19, 2024

broxigarchen commented Oct 24, 2024

Sisyph left a comment

Sisyph commented Oct 28, 2024

kosarev left a comment

kosarev Oct 30, 2024

broxigarchen Oct 30, 2024

[AMDGPU][test]added unique and sort options for update_mc_test_check script #111769

[AMDGPU][test]added unique and sort options for update_mc_test_check script #111769

Conversation

broxigarchen commented Oct 9, 2024 • edited Loading

llvmbot commented Oct 10, 2024 • edited Loading

arichardson left a comment

Choose a reason for hiding this comment

broxigarchen commented Oct 10, 2024 • edited Loading

kosarev left a comment

Choose a reason for hiding this comment

kosarev Oct 11, 2024

Choose a reason for hiding this comment

broxigarchen Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2024 • edited Loading

broxigarchen commented Oct 18, 2024

broxigarchen commented Oct 19, 2024

broxigarchen commented Oct 24, 2024

Sisyph left a comment

Choose a reason for hiding this comment

Sisyph commented Oct 28, 2024

kosarev left a comment

Choose a reason for hiding this comment

kosarev Oct 30, 2024

Choose a reason for hiding this comment

broxigarchen Oct 30, 2024

Choose a reason for hiding this comment

broxigarchen commented Oct 9, 2024 •

edited

Loading

llvmbot commented Oct 10, 2024 •

edited

Loading

broxigarchen commented Oct 10, 2024 •

edited

Loading

broxigarchen Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading