[Utils] Allow update_test_checks to check function arguments

[jdoerfert]

Summary

This adds a switch to the update_test_checks that triggers arguments to
be present in the check line. If not set, the behavior should be the
same as before.

[lebedev.ri]

This doesn't change the default, right?

[xbolva00]

llvm/utils/UpdateTestChecks/common.py

121

if 'args' in m.groupdict():
        if record_args:
             args = m.group('args').strip()
        else:
             args = '('

[jdoerfert]

Include personality, fix versioning based on different args, tested on D68766

[jdoerfert]

In D68819#1704491, @lebedev.ri wrote:

This doesn't change the default, right?

It shouldn't no. I tested it on some formatted files but I'm unsure if there are other interactions I don't know about.

@xbolva00 I don't understand but I updated the version, please take a look

[greened]

Does this subsume the goal of D68153? If so I am happy to abandon that revision. D68153 attempts to solve the problem of a CHECK-LABEL matching a function call instead of the start of a function definition. It looks like with --function-signature the CHECK-LABEL will include the arguments in the label pattern which should be enough to disambiguate it from a call to the function. Do I have that right?

EDIT: Actually, I don't think it will completely work if there is a recursive call to, say, foo that passes the same arguments through. In that case the call will look exactly like the function signature. The same is true if an unrelated function makes a call to foo with values that just happen to be named the same as foo's arguments.

[jdoerfert]

In D68819#1704693, @greened wrote:

Does this subsume the goal of D68153? If so I am happy to abandon that revision. D68153 attempts to solve the problem of a CHECK-LABEL matching a function call instead of the start of a function definition. It looks like with --function-signature the CHECK-LABEL will include the arguments in the label pattern which should be enough to disambiguate it from a call to the function. Do I have that right?

EDIT: Actually, I don't think it will completely work if there is a recursive call to, say, foo that passes the same arguments through. In that case the call will look exactly like the function signature. The same is true if an unrelated function makes a call to foo with values that just happen to be named the same as foo's arguments.

We can, or should, combine D68153 and this, either in one or two patches.

[jdoerfert]

Lessons learned by running it on all argument promotion files: D68766

[greened]

In D68819#1704962, @jdoerfert wrote:

We can, or should, combine D68153 and this, either in one or two patches.

Sounds good. Do you think D68153 should operate under the --function-signature flag, under a different flag or always include define in the pattern (meaning all tests will change when the tool is re-run)?

[jdoerfert]

In D68819#1705424, @greened wrote:

In D68819#1704962, @jdoerfert wrote:

We can, or should, combine D68153 and this, either in one or two patches.

Sounds good. Do you think D68153 should operate under the --function-signature flag, under a different flag or always include define in the pattern (meaning all tests will change when the tool is re-run)?

I think D68153 is a small enough change and at the same time helpful enough to run it always. Adding function signature is not for everyone so I added the flag. I can put it under the flag if people think it should live there though.

[jdoerfert]

ping

[greened]

LGTM. This is useful functionality.

[davezarzycki]

This seems to have broke update_llc_test_checks.py. Is there an easy fix?

Traceback (most recent call last):
  File "./llvm/utils/update_llc_test_checks.py", line 214, in <module>
    main()
  File "./llvm/utils/update_llc_test_checks.py", line 155, in main
    triple, prefixes, func_dict)
  File "/home/dave/s/l/llvm/utils/UpdateTestChecks/asm.py", line 357, in build_function_body_dictionary_for_triple
    func_dict, args.verbose)
TypeError: build_function_body_dictionary() missing 1 required positional argument: 'record_args'

[xbolva00]

Just general note:

Since in recent time there were multiple patches for update scripts, should we consider proper test suite for these scripts?

[arichardson]

It also seems to break update_cc_test_checks.py

[nikic]

Even if --function-signature is not present, this generates spurious diffs:

-; CHECK-LABEL: @test1(
+; CHECK-LABEL: define {{[^@]+}}@test1(

[nikic]

Ugh, I now see that this is actually intentional. I think any changes to the default behavior of update_tests_checks should be discussed on llvm-dev in the future. These kinds of changes are quite disruptive and in this case imho don't pay for themselves.

[jdoerfert]

In D68819#1729354, @nikic wrote:

Ugh, I now see that this is actually intentional. I think any changes to the default behavior of update_tests_checks should be discussed on llvm-dev in the future. These kinds of changes are quite disruptive and in this case imho don't pay for themselves.

It prevents us from accidentally matching the wrong thing, see D68153. We can (now), if we really want to, put that change under the flag as well. I really don't think it should be under a flag. While I see the cost of having new define {{[^@]+}} in upcoming diffs, it will actually improve the match quality. Now if that is worth it is obviously up for debate.

FWIW, D68850 will also change "the default" but only if the default would not have allowed to use the test script in the first place.

[lebedev.ri]

In D68819#1729354, @nikic wrote:

Ugh, I now see that this is actually intentional. I think any changes to the default behavior of update_tests_checks should be discussed on llvm-dev in the future. These kinds of changes are quite disruptive and in this case imho don't pay for themselves.

+1

I even asked about this:

In D68819#1704550, @jdoerfert wrote:

In D68819#1704491, @lebedev.ri wrote:

This doesn't change the default, right?

It shouldn't no. I tested it on some formatted files but I'm unsure if there are other interactions I don't know about.

And was under impression that it was still the case as per

In D68819#1706251, @jdoerfert wrote:

In D68819#1705424, @greened wrote:

In D68819#1704962, @jdoerfert wrote:

We can, or should, combine D68153 and this, either in one or two patches.

Sounds good. Do you think D68153 should operate under the --function-signature flag, under a different flag or always include define in the pattern (meaning all tests will change when the tool is re-run)?

I think D68153 is a small enough change and at the same time helpful enough to run it always. Adding function signature is not for everyone so I added the flag. I can put it under the flag if people think it should live there though.

@jdoerfert is this really fully intentional?

[jdoerfert]

In D68819#1728480, @arichardson wrote:

It also seems to break update_cc_test_checks.py

Yes, the breakage of the other update scripts was fixed in 4de09e0f4460.

[jdoerfert]

In D68819#1728479, @xbolva00 wrote:

Just general note:

Since in recent time there were multiple patches for update scripts, should we consider proper test suite for these scripts?

yes, please.

[jdoerfert]

I said it before (in some thread) but I think actual improvements to the update scripts should not be delayed or hidden behind flags even if they cause some churn. We can always rerun the script on all files in a separate commit. This actually helps to discover other problems that sneak in over time anyway. As an example I rerun this on test/Analysis in D69687.

[spatel]

Here's a current example of the trade-off:
D59710

I agree that adding the 'define...' glob to the label check is an improvement, but at the same time, many of us use the scripts almost exclusively on single function regression tests (no calls)...which is probably why we never encountered the motivating problem for this change.

As I understand it, there's currently no way to disable the 'define' glob. So I either have the burden of doing separate pre-commits to update hundreds of test files or ask reviewers to look past that noise. We want to push people towards the better future, but it would be more friendly to offer this feature with an opt-out. That is, default to the new way, but let people disable the 'define' generation to update on their own terms/as needed.

[xbolva00]

Can the script rather somehow detect and warn for mentioned problematic cases than making churn? It can suggest “Problem detected. Use flag —XXX to emit precise checks”.

We should be careful how often we change default behaviour and if such thing is really needed.
Somebody in the future may again change the script -> new churn -> productivity decreases..

[xbolva00]

Or detect if no issue and emit old checks. No churn too.

[jdoerfert]

In D68819#1730164, @spatel wrote:

Here's a current example of the trade-off:
D59710

I agree that adding the 'define...' glob to the label check is an improvement, but at the same time, many of us use the scripts almost exclusively on single function regression tests (no calls)...which is probably why we never encountered the motivating problem for this change.

As I understand it, there's currently no way to disable the 'define' glob. So I either have the burden of doing separate pre-commits to update hundreds of test files or ask reviewers to look past that noise. We want to push people towards the better future, but it would be more friendly to offer this feature with an opt-out. That is, default to the new way, but let people disable the 'define' generation to update on their own terms/as needed.

The combination of D69701 and D69719 will remove the unwanted effects but allow proper support in the future. If we get them in before your commit the churn goes away.

[spatel]

In D68819#1730310, @jdoerfert wrote:

In D68819#1730164, @spatel wrote:

Here's a current example of the trade-off:
D59710

I agree that adding the 'define...' glob to the label check is an improvement, but at the same time, many of us use the scripts almost exclusively on single function regression tests (no calls)...which is probably why we never encountered the motivating problem for this change.

As I understand it, there's currently no way to disable the 'define' glob. So I either have the burden of doing separate pre-commits to update hundreds of test files or ask reviewers to look past that noise. We want to push people towards the better future, but it would be more friendly to offer this feature with an opt-out. That is, default to the new way, but let people disable the 'define' generation to update on their own terms/as needed.

The combination of D69701 and D69719 will remove the unwanted effects but allow proper support in the future. If we get them in before your commit the churn goes away.

Thanks! I haven't followed all of the script patches, but things are quiet once again in the files that I've updated just now.

llvm/utils/UpdateTestChecks/common.py

 ##### LLVM IR parser
 
 RUN_LINE_RE = re.compile('^\s*[;#]\s*RUN:\s*(.*)$')
 CHECK_PREFIX_RE = re.compile('--?check-prefix(?:es)?[= ](\S+)')
 PREFIX_RE = re.compile('^[a-zA-Z0-9_-]+$')
 CHECK_RE = re.compile(r'^\s*[;#]\s*([^:]+?)(?:-NEXT|-NOT|-DAG|-LABEL)?:')
 
 OPT_FUNCTION_RE = re.compile(
-    r'^\s*define\s+(?:internal\s+)?[^@]*@(?P<func>[\w-]+?)\s*\('
-    r'(\s+)?[^)]*[^{]*\{\n(?P<body>.*?)^\}$',
+    r'^\s*define\s+(?:internal\s+)?[^@]*@(?P<func>[\w-]+?)\s*'
+    r'(?P<args_and_sig>\((\s+)?[^)]*\)[^{]*?)\{\n(?P<body>.*?)^\}$',
     flags=(re.M | re.S))
 
 ANALYZE_FUNCTION_RE = re.compile(
     r'^\s*\'(?P<analysis>[\w\s-]+?)\'\s+for\s+function\s+\'(?P<func>[\w-]+?)\':'
     r'\s*\n(?P<body>.*)$',
     flags=(re.X | re.S))
 
 IR_FUNCTION_RE = re.compile('^\s*define\s+(?:internal\s+)?[^@]*@(\w+)\s*\(')
def do_scrub(body, scrubber, scrubber_args, extra):
   if scrubber_args:
     local_args = copy.deepcopy(scrubber_args)
     local_args[0].extra_scrub = extra
     return scrubber(body, *local_args)
   return scrubber(body, *scrubber_args)
 
 # Build up a dictionary of all the function bodies.
 class function_body(object):
-  def __init__(self, string, extra):
+  def __init__(self, string, extra, args_and_sig):
     self.scrub = string
     self.extrascrub = extra
+    self.args_and_sig = args_and_sig
   def __str__(self):
     return self.scrub
 
-def build_function_body_dictionary(function_re, scrubber, scrubber_args, raw_tool_output, prefixes, func_dict, verbose):
+def build_function_body_dictionary(function_re, scrubber, scrubber_args, raw_tool_output, prefixes, func_dict, verbose, record_args):
   for m in function_re.finditer(raw_tool_output):
     if not m:
       continue
     func = m.group('func')
     body = m.group('body')
+    # Determine if we print arguments, the opening brace, or nothing after the function name
+    if record_args and 'args_and_sig' in m.groupdict():
+        args_and_sig = m.group('args_and_sig').strip()
+    elif 'args_and_sig' in m.groupdict():
+        args_and_sig = '('
+    else:
+        args_and_sig = ''
     scrubbed_body = do_scrub(body, scrubber, scrubber_args, extra = False)
     scrubbed_extra = do_scrub(body, scrubber, scrubber_args, extra = True)
     if 'analysis' in m.groupdict():
       analysis = m.group('analysis')
       if analysis.lower() != 'cost model analysis':
         warn('Unsupported analysis mode: %r!' % (analysis,))
     if func.startswith('stress'):
       # We only use the last line of the function body for stress tests.
       scrubbed_body = '\n'.join(scrubbed_body.splitlines()[-1:])
     if verbose:
       print('Processing function: ' + func, file=sys.stderr)
       for l in scrubbed_body.splitlines():
         print('  ' + l, file=sys.stderr)
     for prefix in prefixes:
-      if func in func_dict[prefix] and str(func_dict[prefix][func]) != scrubbed_body:
-        if func_dict[prefix][func] and func_dict[prefix][func].extrascrub == scrubbed_extra:
+      if func in func_dict[prefix] and (str(func_dict[prefix][func]) != scrubbed_body or func_dict[prefix][func].args_and_sig != args_and_sig):
+        if func_dict[prefix][func] and func_dict[prefix][func].extrascrub == scrubbed_extra and func_dict[prefix][func].args_and_sig == args_and_sig:
           func_dict[prefix][func].scrub = scrubbed_extra
           continue
         else:
           if prefix == prefixes[-1]:
             warn('Found conflicting asm under the same prefix: %r!' % (prefix,))
           else:
             func_dict[prefix][func] = None
             continue
 
-      func_dict[prefix][func] = function_body(scrubbed_body, scrubbed_extra)
+      func_dict[prefix][func] = function_body(scrubbed_body, scrubbed_extra, args_and_sig)
 
 ##### Generator of LLVM IR CHECK lines
 
 SCRUB_IR_COMMENT_RE = re.compile(r'\s*;.*')
 
 # Match things that look at identifiers, but only if they are followed by
 # spaces, commas, paren, or end of the string
 IR_VALUE_RE = re.compile(r'(\s+)%([\w\.\-]+?)([,\s\(\)]|\Z)')
for checkprefix in checkprefixes:
 
       # Add some space between different check prefixes, but not after the last
       # check line (before the test code).
       if is_asm:
         if len(printed_prefixes) != 0:
           output_lines.append(comment_marker)
 
       printed_prefixes.append(checkprefix)
-      output_lines.append(check_label_format % (checkprefix, func_name))
+      args_and_sig = str(func_dict[checkprefix][func_name].args_and_sig)
+      output_lines.append(check_label_format % (checkprefix, func_name, args_and_sig))
       func_body = str(func_dict[checkprefix][func_name]).splitlines()
 
       # For ASM output, just emit the check lines.
       if is_asm:
         output_lines.append('%s %s:       %s' % (comment_marker, checkprefix, func_body[0]))
         for func_line in func_body[1:]:
           output_lines.append('%s %s-NEXT:  %s' % (comment_marker, checkprefix, func_line))
         break
for checkprefix in checkprefixes:
       # Add space between different check prefixes and also before the first
       # line of code in the test function.
       output_lines.append(comment_marker)
       break
 
 def add_ir_checks(output_lines, comment_marker, prefix_list, func_dict,
                   func_name, preserve_names):
   # Label format is based on IR string.
-  check_label_format = '{} %s-LABEL: @%s('.format(comment_marker)
+  check_label_format = '{} %s-LABEL: @%s%s'.format(comment_marker)
   add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name,
              check_label_format, False, preserve_names)
 
 def add_analyze_checks(output_lines, comment_marker, prefix_list, func_dict, func_name):
-  check_label_format = '{} %s-LABEL: \'%s\''.format(comment_marker)
+  check_label_format = '{} %s-LABEL: \'%s%s\''.format(comment_marker)
   add_checks(output_lines, comment_marker, prefix_list, func_dict, func_name, check_label_format, False, True)
 
 
 def check_prefix(prefix):
   if not PREFIX_RE.match(prefix):
         hint = ""
         if ',' in prefix:
           hint = " Did you mean '--check-prefixes=" + prefix + "'?"

llvm/utils/update_analyze_test_checks.py

for prefixes, opt_args in prefix_list:
         print('Extracted FileCheck prefixes: ' + str(prefixes), file=sys.stderr)
 
       raw_tool_outputs = common.invoke_tool(args.opt_binary, opt_args, test)
 
       # Split analysis outputs by "Printing analysis " declarations.
       for raw_tool_output in re.split(r'Printing analysis ', raw_tool_outputs):
         common.build_function_body_dictionary(
           common.ANALYZE_FUNCTION_RE, common.scrub_body, [],
-          raw_tool_output, prefixes, func_dict, args.verbose)
+          raw_tool_output, prefixes, func_dict, args.verbose, False)
 
     is_in_function = False
     is_in_function_start = False
     prefix_set = set([prefix for prefixes, _ in prefix_list for prefix in prefixes])
     if args.verbose:
       print('Rewriting FileCheck prefixes: %s' % (prefix_set,), file=sys.stderr)
     output_lines = []
     output_lines.append(autogenerated_note)

llvm/utils/update_mir_test_checks.py

for prefixes, llc_args, triple_in_cmd in run_list:
 
         raw_tool_output = args.llc(llc_args, test)
         if not triple_in_cmd and not triple_in_ir:
             common.warn('No triple found: skipping file', test_file=test)
             return
 
         build_function_body_dictionary(test, raw_tool_output,
                                        triple_in_cmd or triple_in_ir,
-                                       prefixes, func_dict, args.verbose)
+                                       prefixes, func_dict, args.verbose, False)
 
     state = 'toplevel'
     func_name = None
     prefix_set = set([prefix for run in run_list for prefix in run.prefixes])
     log('Rewriting FileCheck prefixes: {}'.format(prefix_set), args.verbose)
 
     if args.remove_common_prefixes:
         prefix_set.update(common_prefixes)

llvm/utils/update_test_checks.py

def main():
   parser.add_argument('--opt-binary', default='opt',
                       help='The opt binary used to generate the test case')
   parser.add_argument(
       '--function', help='The function in the test file to update')
   parser.add_argument('-u', '--update-only', action='store_true',
                       help='Only update test if it was already autogened')
   parser.add_argument('-p', '--preserve-names', action='store_true',
                       help='Do not scrub IR names')
+  parser.add_argument('--function-signature', action='store_true',
+                      help='Keep function signature information around for the check line')
   parser.add_argument('tests', nargs='+')
   args = parser.parse_args()
 
   script_name = os.path.basename(__file__)
   autogenerated_note = (ADVERT + 'utils/' + script_name)
 
   opt_basename = os.path.basename(args.opt_binary)
   if not re.match(r'^opt(-\d+)?$', opt_basename):
for test in test_paths:
     for prefixes, opt_args in prefix_list:
       if args.verbose:
         print('Extracted opt cmd: ' + opt_basename + ' ' + opt_args, file=sys.stderr)
         print('Extracted FileCheck prefixes: ' + str(prefixes), file=sys.stderr)
 
       raw_tool_output = common.invoke_tool(args.opt_binary, opt_args, test)
       common.build_function_body_dictionary(
               common.OPT_FUNCTION_RE, common.scrub_body, [],
-              raw_tool_output, prefixes, func_dict, args.verbose)
+              raw_tool_output, prefixes, func_dict, args.verbose,
+              args.function_signature)
 
     is_in_function = False
     is_in_function_start = False
     prefix_set = set([prefix for prefixes, _ in prefix_list for prefix in prefixes])
     if args.verbose:
       print('Rewriting FileCheck prefixes: %s' % (prefix_set,), file=sys.stderr)
     output_lines = []
     output_lines.append(autogenerated_note)