- 
                Notifications
    You must be signed in to change notification settings 
- Fork 15k
[utils][UpdateLLCTestChecks] Add MIR support to update_llc_test_checks.py. #164965
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
base: main
Are you sure you want to change the base?
Conversation
| @llvm/pr-subscribers-testing-tools Author: Valery Pykhtin (vpykhtin) ChangesThis change enables update_llc_test_checks.py to automatically generate MIR checks for RUN lines that use  This resulted from the scenario, when I needed to test two instruction matching patterns where the later pattern in the peepholler reverts the earlier pattern in the instruction selector and distinguish it from the case when the earlier pattern didn't worked at all. Drawback: this may create very noisy tests. Note: This is 100% Claude Sonnet 4.5 and I don't feel I understand the change in details, but the change looks so small that I decided to give it a chance. Full diff: https://github.com/llvm/llvm-project/pull/164965.diff 1 Files Affected: 
 diff --git a/llvm/utils/update_llc_test_checks.py b/llvm/utils/update_llc_test_checks.py
index 8c57e75f34f75..5c008e2d2ed2c 100755
--- a/llvm/utils/update_llc_test_checks.py
+++ b/llvm/utils/update_llc_test_checks.py
@@ -13,9 +13,11 @@
 from traceback import print_exc
 import argparse
 import os  # Used to advertise this file's name ("autogenerated_note").
+import re
 import sys
 
 from UpdateTestChecks import common
+import update_mir_test_checks # Reuse MIR parsing code.
 
 # llc is the only llc-like in the LLVM tree but downstream forks can add
 # additional ones here if they have them.
@@ -23,6 +25,9 @@
     "llc",
 ]
 
+def has_stop_pass_flag(llc_args):
+    """Check if llc arguments contain -stop-before or -stop-after flag."""
+    return re.search(r'-stop-(?:before|after)=', llc_args) is not None
 
 def update_test(ti: common.TestInfo):
     triple_in_ir = None
@@ -119,6 +124,10 @@ def update_test(ti: common.TestInfo):
         ginfo=ginfo,
     )
 
+    # Dictionary to store MIR function bodies separately
+    mir_func_dict = {}
+    mir_run_list = []
+
     for (
         prefixes,
         llc_tool,
@@ -141,14 +150,31 @@ def update_test(ti: common.TestInfo):
         if not triple:
             triple = common.get_triple_from_march(march_in_cmd)
 
-        scrubber, function_re = output_type.get_run_handler(triple)
-        if 0 == builder.process_run_line(
-            function_re, scrubber, raw_tool_output, prefixes
-        ):
-            common.warn(
-                "Couldn't match any function. Possibly the wrong target triple has been provided"
+        # Check if this run generates MIR output
+        if has_stop_pass_flag(llc_args):
+            common.debug("Detected MIR output mode for prefixes:", str(prefixes))
+            # Initialize MIR func_dict for these prefixes.
+            for prefix in prefixes:
+                if prefix not in mir_func_dict:
+                    mir_func_dict[prefix] = {}
+            
+            # Build MIR function dictionary using imported function.
+            update_mir_test_checks.build_function_info_dictionary(
+                ti.path, raw_tool_output, triple, prefixes, mir_func_dict, ti.args.verbose
             )
-        builder.processed_prefixes(prefixes)
+            
+            # Store this run info for later.
+            mir_run_list.append((prefixes, llc_tool, llc_args, triple_in_cmd, march_in_cmd))
+        else:
+            # Regular assembly output.
+            scrubber, function_re = output_type.get_run_handler(triple)
+            if 0 == builder.process_run_line(
+                function_re, scrubber, raw_tool_output, prefixes
+            ):
+                common.warn(
+                    "Couldn't match any function. Possibly the wrong target triple has been provided"
+                )
+            builder.processed_prefixes(prefixes)
 
     func_dict = builder.finish_and_get_func_dict()
     global_vars_seen_dict = {}
@@ -221,6 +247,21 @@ def update_test(ti: common.TestInfo):
                         is_filtered=builder.is_filtered(),
                     )
                 )
+                
+                # Also add MIR checks if we have them for this function
+                if mir_run_list and func_name:
+                    common.add_mir_checks_for_function(
+                        ti.path,
+                        output_lines,
+                        mir_run_list,
+                        mir_func_dict,
+                        func_name,
+                        single_bb=False, # Don't skip basic block labels.
+                        print_fixed_stack=False, # Don't print fixed stack (ASM tests don't need it).
+                        first_check_is_next=False, # First check is LABEL, not NEXT.
+                        at_the_function_name=False, # Use "name:" not "@name".
+                    )
+                
                 is_in_function_start = False
 
             if is_in_function:
 | 
| ✅ With the latest revision this PR passed the Python code formatter. | 
c76fa54    to
    9d05987      
    Compare
  
    | Ok, this has failed testing, going to fix and add new one. | 
6bdef82    to
    5f596b6      
    Compare
  
            
          
                llvm/utils/update_llc_test_checks.py
              
                Outdated
          
        
      | import sys | ||
|  | ||
| from UpdateTestChecks import common | ||
| import update_mir_test_checks # Reuse MIR parsing code. | 
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.
Could you move the functions you used here into common instead of importing from the script?
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.
Thanks, done. Probably would be nice to move MIR specific code to the new mir.py, part of MIR code was already moved into common in #140296.
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 decided to separate the move into a separate commit and rebase this PR on top of it.
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.
Two minor comments otherwise LGTM. Thanks
        
          
                llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/x86_asm_mir_mixed.ll.expected
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | Maybe naive question, but for your scenario, can you run just the passes you need, on MIR? I.e you could start with the pattern that you expect to be undone, and not rely on the earlier pass producing it. You'd also not need this change then. ... which would avoid teasing the sentiments in this thread: https://discourse.llvm.org/t/rfc-llvm-ai-tool-policy-start-small-no-slop/88476 (I'm assuming @arichardson's lgtm means at least one person is ok with maintaining Claude's output here) | 
| 
 I definitely can do this, and also I can add manually created checks in my scenario (like grepping for a specific instruction presence), we already have similar test in our code base. Ok, let it sit here for a while. Probably the main question for me here if such functionality is desired at all. | 
| 
 The reason I approved this is that the change looks trivial and I have had cases in the past where I wanted to test the llc output both after a specific pass (usually ISel) as well as the final assembly. Not having to have two tests that could get out of sync is quite nice there but of course you could also use manual check lines or add a MIR test. | 
| This PR actually came up at the LLVM developer meeting round table on AI generated contributions (notes), and I said this sounds like a totally appropriate use for LLMs. The underlying principle behind the policy has been "how do we ensure that users are not wasting maintainer time by offloading the work of reviewing LLM generated code onto maintainers?" When it comes to Python utility scripts in the repository, I don't think that maintainers are going through these scripts with a fine-toothed comb to ensure that they pose no threat to the correctness and stability of LLVM compilers, so I think we can say clearly that this is not wasting maintainer time and is an encouraged of LLMs. | 
….py module (#165535) This commit extracts some MIR-related code from `common.py` and `update_mir_test_checks.py` into a dedicated `mir.py` module to improve code organization. This is a preparation step for #164965 and also moves some pieces already moved by #140296 All code intentionally moved verbatim with minimal necessary adaptations: * `log()` calls converted to `print(..., file=sys.stderr)` at `mir.py` lines 62, 64 due to a `log` locality.
2d34e04    to
    74e3a74      
    Compare
  
    | Rebased and squashed. After some testing I made additional changes: 
 Check indentation looks consistent with ASM checks. | 
…s.py This change enables update_llc_test_checks.py to automatically generate MIR checks for RUN lines that use -stop-before or -stop-after flags. This allows tests to verify intermediate compilation stages (e.g., after instruction selection but before peephole optimizations) alongside the final assembly output. Remove extra indentation from MIR check lines conflict processing
74e3a74    to
    0efffe4      
    Compare
  
    …eparate mir.py module (#165535) This commit extracts some MIR-related code from `common.py` and `update_mir_test_checks.py` into a dedicated `mir.py` module to improve code organization. This is a preparation step for llvm/llvm-project#164965 and also moves some pieces already moved by llvm/llvm-project#140296 All code intentionally moved verbatim with minimal necessary adaptations: * `log()` calls converted to `print(..., file=sys.stderr)` at `mir.py` lines 62, 64 due to a `log` locality.
….py module (llvm#165535) This commit extracts some MIR-related code from `common.py` and `update_mir_test_checks.py` into a dedicated `mir.py` module to improve code organization. This is a preparation step for llvm#164965 and also moves some pieces already moved by llvm#140296 All code intentionally moved verbatim with minimal necessary adaptations: * `log()` calls converted to `print(..., file=sys.stderr)` at `mir.py` lines 62, 64 due to a `log` locality.
|  | ||
| define i64 @test1(i64 %i) nounwind readnone { | ||
| %loc = alloca i64 | ||
| %j = load i64, i64 * %loc | 
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.
Use opaque pointers
| ; MIR-NEXT: $rax = COPY [[ADD64rm]] | ||
| ; MIR-NEXT: RET 0, $rax | ||
| %loc = alloca i64 | ||
| %j = load i64, i64 * %loc | 
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.
Opaque pointers
This change enables update_llc_test_checks.py to automatically generate MIR checks for RUN lines that use
-stop-beforeor-stop-afterflags allowing tests to verify intermediate compilation stages (e.g., after instruction selection but before peephole optimizations) alongside the final assembly output.This resulted from the scenario, when I needed to test two instruction matching patterns where the later pattern in the peepholler reverts the earlier pattern in the instruction selector and distinguish it from the case when the earlier pattern didn't worked at all.
Drawback: this may create very noisy tests.
Note: This is 100% Claude Sonnet 4.5 and I don't feel I understand the change in details, but the change looks so small that I decided to give it a chance.
Example: