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]added unique and sort options for update_mc_test_check script #111769

Merged

Conversation

broxigarchen
Copy link
Contributor

@broxigarchen broxigarchen commented Oct 9, 2024

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

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

@broxigarchen 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 broxigarchen changed the title [AMDGPU][test]added remove dup options [AMDGPU][test]added remove duplicate options Oct 9, 2024
@broxigarchen 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 broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from ac8ee83 to 4756c55 Compare October 10, 2024 05:28
@broxigarchen broxigarchen marked this pull request as ready for review October 10, 2024 05:29
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 10, 2024

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

@broxigarchen 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
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.

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 broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from 26f9d92 to 98ece71 Compare October 10, 2024 23:54
@broxigarchen
Copy link
Contributor Author

broxigarchen commented Oct 10, 2024

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

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.

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 Outdated Show resolved Hide resolved
llvm/utils/update_mc_test_checks.py Outdated Show resolved Hide resolved
llvm/utils/update_mc_test_checks.py Show resolved Hide resolved
parser.add_argument(
"--sort",
action="store_true",
default=False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@broxigarchen broxigarchen Oct 11, 2024

Choose a reason for hiding this comment

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

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

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

github-actions bot commented Oct 17, 2024

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

@broxigarchen broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from 13febff to 107d161 Compare October 17, 2024 15:01
@broxigarchen 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
Copy link
Contributor Author

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

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

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

ping!

broxigarchen added a commit that referenced this pull request Oct 28, 2024
)

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

@Sisyph Sisyph left a comment

Choose a reason for hiding this comment

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

LGTM, but please wait for @kosarev

@Sisyph
Copy link
Contributor

Sisyph commented Oct 28, 2024

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.

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 with nits.

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
@broxigarchen broxigarchen force-pushed the update-mc-check-script-with-remove-dup branch from 0eb494c to 788194d Compare October 29, 2024 15:25
@broxigarchen broxigarchen merged commit 528e975 into llvm:main Oct 29, 2024
8 checks passed
@@ -6,6 +6,7 @@
from __future__ import print_function

import argparse
import functools
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unused now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this in another patch

broxigarchen added a commit that referenced this pull request Oct 30, 2024
…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
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.

6 participants