[UTC] Enable --function-signature by default

[nikic]

Summary

This patch enables --function-signature by default under --version 2 and makes --version 2 the default. This means that all newly created tests will check the function signature, while leaving old tests alone.

There's two motivations for this change:

• Without --function-signature, the generated check lines may fail in a very hard to understand way if the test both includes a function definition and a call to that function. (Though we could address this by making the CHECK-LABEL stricter, without checking the full signature.)
• This actually checks that uses of the arguments in the function body use the correct argument, instead of matching against any variable.

This is a replacement for D139006 and D140212 based on the --version mechanism.

I did not include an opt-out flag --no-function-signature because I'm not sure we need it. Would be happy to include it though, if desired.

[spatel]

I didn't follow the reviews of D142473 and D144963 while they were in progress, but this is the logical conclusion to the set. LGTM.

[arichardson]

llvm/utils/UpdateTestChecks/common.py

252

I think this would be the place to add if args.version >= 2: args.function_signature = True

llvm/utils/update_cc_test_checks.py

352

Could we update function_signature to True if version == 2 after parsing? That would avoid the need for the new helper function.

[arichardson]

Thanks for working on this! How bad is the test diff if you remove the hardcoded --version=1 from lit.local.cfg?

[nikic]

In D145149#4168583, @arichardson wrote:

Thanks for working on this! How bad is the test diff if you remove the hardcoded --version=1 from lit.local.cfg?

Beyond the actual functional diff, this will touch every single UTC test simply due to the addition of --version 2 to UTC_ARGS. I'd need an update_utc_test_checks.py script for that ;)

llvm/utils/UpdateTestChecks/common.py

252

That wouldn't be sufficient due to the on-the-fly flags update feature (that probably isn't worth the complexity it introduces -- we should consider removing it.) I could extract the parse_args() calls into a common helper though, where we could do this.

[nikic]

Adjust args.function_signature instead of adding a helper to derive the correct value.

[arichardson]

Thanks, LGTM

clang/test/utils/update_cc_test_checks/Inputs/mangled_names.c.v2.expected

+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2
+// Example input for update_cc_test_checks
+// RUN: %clang_cc1 -triple=x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+// CHECK-LABEL: define dso_local i64 @test
+// CHECK-SAME: (i64 noundef [[A:%.*]], i32 noundef [[B:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
+// CHECK-NEXT:    [[B_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i64 [[A]], ptr [[A_ADDR]], align 8
+// CHECK-NEXT:    store i32 [[B]], ptr [[B_ADDR]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load i64, ptr [[A_ADDR]], align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr [[B_ADDR]], align 4
+// CHECK-NEXT:    [[CONV:%.*]] = sext i32 [[TMP1]] to i64
+// CHECK-NEXT:    [[ADD:%.*]] = add nsw i64 [[TMP0]], [[CONV]]
+// CHECK-NEXT:    ret i64 [[ADD]]
+//
+long test(long a, int b) {
+  return a + b;
+}
+
+// A function with a mangled name
+// CHECK-LABEL: define dso_local i64 @_Z4testlii
+// CHECK-SAME: (i64 noundef [[A:%.*]], i32 noundef [[B:%.*]], i32 noundef [[C:%.*]]) #[[ATTR0]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[A_ADDR:%.*]] = alloca i64, align 8
+// CHECK-NEXT:    [[B_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    [[C_ADDR:%.*]] = alloca i32, align 4
+// CHECK-NEXT:    store i64 [[A]], ptr [[A_ADDR]], align 8
+// CHECK-NEXT:    store i32 [[B]], ptr [[B_ADDR]], align 4
+// CHECK-NEXT:    store i32 [[C]], ptr [[C_ADDR]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = load i64, ptr [[A_ADDR]], align 8
+// CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr [[B_ADDR]], align 4
+// CHECK-NEXT:    [[CONV:%.*]] = sext i32 [[TMP1]] to i64
+// CHECK-NEXT:    [[ADD:%.*]] = add nsw i64 [[TMP0]], [[CONV]]
+// CHECK-NEXT:    [[TMP2:%.*]] = load i32, ptr [[C_ADDR]], align 4
+// CHECK-NEXT:    [[CONV1:%.*]] = sext i32 [[TMP2]] to i64
+// CHECK-NEXT:    [[ADD2:%.*]] = add nsw i64 [[ADD]], [[CONV1]]
+// CHECK-NEXT:    ret i64 [[ADD2]]
+//
+__attribute__((overloadable)) long test(long a, int b, int c) {
+  return a + b + c;
+}

clang/test/utils/update_cc_test_checks/mangled_names.test

 ## Verify that running without the --function-signature flag removes the -SAME: lines:
 ## We have to remove the UTC_ARGS comment first:
 # RUN: grep -v UTC_ARGS %t.c > %t-no-args.c
 # RUN: %update_cc_test_checks %t-no-args.c
 # RUN: diff -u %t-no-args.c %S/Inputs/mangled_names.c.expected
 ## Also try --version 2
 # RUN: %update_cc_test_checks %t.c --function-signature --version 2
 # RUN: diff -u %t.c %S/Inputs/mangled_names.c.funcsig.v2.expected
+## Restore the original file without --function-signature and check that
+## --version 2 will implicitly enable it.
+# RUN: cp -f %S/Inputs/mangled_names.c %t.c
+# RUN: %update_cc_test_checks %t.c --version 2
+# RUN: diff -u %t.c %S/Inputs/mangled_names.c.v2.expected

llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/basic.ll.v2.expected

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; Example input for update_test_checks (taken from test/Transforms/InstSimplify/add.ll)
+; RUN: opt < %s -passes=instsimplify -S | FileCheck %s
+
+define i32 @common_sub_operand(i32 %X, i32 %Y) {
+; CHECK-LABEL: define i32 @common_sub_operand
+; CHECK-SAME: (i32 [[X:%.*]], i32 [[Y:%.*]]) {
+; CHECK-NEXT:    ret i32 [[X]]
+;
+  %Z = sub i32 %X, %Y
+  %Q = add i32 %Z, %Y
+  ret i32 %Q
+}
+
+define i32 @negated_operand(i32 %x) {
+; CHECK-LABEL: define i32 @negated_operand
+; CHECK-SAME: (i32 [[X:%.*]]) {
+; CHECK-NEXT:    ret i32 0
+;
+  %negx = sub i32 0, %x
+  %r = add i32 %negx, %x
+  ret i32 %r
+}
+
+define <2 x i32> @negated_operand_commute_vec(<2 x i32> %x) {
+; CHECK-LABEL: define <2 x i32> @negated_operand_commute_vec
+; CHECK-SAME: (<2 x i32> [[X:%.*]]) {
+; CHECK-NEXT:    ret <2 x i32> zeroinitializer
+;
+  %negx = sub <2 x i32> zeroinitializer, %x
+  %r = add <2 x i32> %x, %negx
+  ret <2 x i32> %r
+}
+
+define i8 @knownnegation(i8 %x, i8 %y) {
+; CHECK-LABEL: define i8 @knownnegation
+; CHECK-SAME: (i8 [[X:%.*]], i8 [[Y:%.*]]) {
+; CHECK-NEXT:    ret i8 0
+;
+  %xy = sub i8 %x, %y
+  %yx = sub i8 %y, %x
+  %r = add i8 %xy, %yx
+  ret i8 %r
+}
+
+define <2 x i8> @knownnegation_commute_vec(<2 x i8> %x, <2 x i8> %y) {
+; CHECK-LABEL: define <2 x i8> @knownnegation_commute_vec
+; CHECK-SAME: (<2 x i8> [[X:%.*]], <2 x i8> [[Y:%.*]]) {
+; CHECK-NEXT:    ret <2 x i8> zeroinitializer
+;
+  %xy = sub <2 x i8> %x, %y
+  %yx = sub <2 x i8> %y, %x
+  %r = add <2 x i8> %yx, %xy
+  ret <2 x i8> %r
+}
+
+define i32 @nameless_value(i32 %X) {
+; CHECK-LABEL: define i32 @nameless_value
+; CHECK-SAME: (i32 [[X:%.*]]) {
+; CHECK-NEXT:    [[TMP1:%.*]] = sub i32 42, [[X]]
+; CHECK-NEXT:    ret i32 [[TMP1]]
+;
+  %1 = sub i32 42, %X
+  ret i32 %1
+}

llvm/test/tools/UpdateTestChecks/update_test_checks/basic.test

 # REQUIRES: x86-registered-target
 ## Basic test checking that update_test_checks.py works correctly
 # RUN: cp -f %S/Inputs/basic.ll %t.ll && %update_test_checks %t.ll
 # RUN: diff -u %t.ll %S/Inputs/basic.ll.expected
 ## Check that running the script again does not change the result:
 # RUN: %update_test_checks %t.ll
 # RUN: diff -u %t.ll %S/Inputs/basic.ll.expected
 ## Also try the --function-signature flag
 # RUN: %update_test_checks %t.ll --function-signature
 # RUN: diff -u %t.ll %S/Inputs/basic.ll.funcsig.expected
 ## Verify that running without the --function-signature flag does not removes
 ## the -SAME: lines since the generated file will have --function-signature in
 ## an UTC_ARGS: comment in the first line (from the invocation above) which is
 ## added to the update invocation below.
 # RUN: %update_test_checks %t.ll
 # RUN: diff -u %t.ll %S/Inputs/basic.ll.funcsig.expected
+## Restore the original file without --function-signature and check that
+## --version 2 will implicitly enable it and also check the return type.
+# RUN: cp -f %S/Inputs/basic.ll %t.ll
+# RUN: %update_test_checks %t.ll --version 2
+# RUN: diff -u %t.ll %S/Inputs/basic.ll.v2.expected

llvm/utils/UpdateTestChecks/common.py

 
 _verbose = False
 _prefix_filecheck_ir_name = ''
 
 """
 Version changelog:
 
 1: Initial version, used by tests that don't specify --version explicitly.
-2: --function-signature now also checks return type/attributes.
+2: --function-signature is now enabled by default and also checks return
+   type/attributes.
 """
-DEFAULT_VERSION = 1
+DEFAULT_VERSION = 2
 
 class Regex(object):
   """Wrap a compiled regular expression object to allow deep copy of a regexp.
   This is required for the deep copy done in do_scrub.
 
   """
   def __init__(self, regex):
     self.regex = regex
parser.add_argument('--version', type=int, default=1,
                       help='The version of output format')
   args = parser.parse_args()
   global _verbose, _global_value_regex, _global_hex_value_regex
   _verbose = args.verbose
   _global_value_regex = args.global_value_regex
   _global_hex_value_regex = args.global_hex_value_regex
   return args
 
+def check_func_sig(args):
+  return args.function_signature or args.version > 1
 
 class InputLineInfo(object):
   def __init__(self, line, line_number, args, argv):
     self.line = line
     self.line_number = line_number
     self.args = args
     self.argv = argv
 
for test in tests_list:
       first_line = input_lines[0] if input_lines else ""
       is_regenerate = UTC_ADVERT in first_line
 
       # If we're generating a new test, set the default version to the latest.
       argv = sys.argv[:]
       if not is_regenerate:
         argv.insert(1, '--version=' + str(DEFAULT_VERSION))
 
       args = parser.parse_args(argv[1:])
       if argparse_callback is not None:
         argparse_callback(args)
       if is_regenerate:
         if script_name not in first_line and not args.force_update:
           warn("Skipping test which wasn't autogenerated by " + script_name, test)
           continue
         args, argv = check_for_command(first_line, parser, args, argv, argparse_callback)
       elif args.update_only:
def is_same_except_arg_names(self, extrascrub, funcdef_attrs_and_ret, args_and_sig, attrs, is_backend):
     es0 = SCRUB_IR_COMMENT_RE.sub(r'', es0)
     es1 = SCRUB_IR_COMMENT_RE.sub(r'', es1)
     return es0 == es1
 
   def __str__(self):
     return self.scrub
 
 class FunctionTestBuilder:
-  def __init__(self, run_list, flags, scrubber_args, path):
+  def __init__(self, run_list, flags, scrubber_args, path, record_args):
     self._verbose = flags.verbose
-    self._record_args = flags.function_signature
+    self._record_args = record_args
     self._check_attributes = flags.check_attributes
     # Strip double-quotes if input was read by UTC_ARGS
     self._filters = list(map(lambda f: Filter(re.compile(f.pattern().strip('"'),
                                                          f.flags()),
                                               f.is_filter_out),
                              flags.filters)) if flags.filters else []
     self._scrubber_args = scrubber_args
     self._path = path

llvm/utils/update_analyze_test_checks.py

builder = common.FunctionTestBuilder(
       run_list = prefix_list,
       flags = type('', (object,), {
             'verbose': ti.args.verbose,
             'filters': ti.args.filters,
             'function_signature': False,
             'check_attributes': False,
             'replace_value_regex': []}),
       scrubber_args = [],
-      path=ti.path)
+      path=ti.path,
+      record_args=False)
 
     for prefixes, opt_args in prefix_list:
       common.debug('Extracted opt cmd:', opt_basename, opt_args, file=sys.stderr)
       common.debug('Extracted FileCheck prefixes:', str(prefixes), file=sys.stderr)
 
       raw_tool_outputs = common.invoke_tool(ti.args.opt_binary, opt_args, ti.path)
 
       if re.search(r'Printing analysis ', raw_tool_outputs) is not None:

llvm/utils/update_cc_test_checks.py

for ti in common.itertests(initial_args.tests, parser, 'utils/' + script_name,
     # Execute clang, generate LLVM IR, and extract functions.
 
     # Store only filechecked runlines.
     filecheck_run_list = [i for i in run_list if i[0]]
     builder = common.FunctionTestBuilder(
       run_list=filecheck_run_list,
       flags=ti.args,
       scrubber_args=[],
-      path=ti.path)
+      path=ti.path,
+      record_args=common.check_func_sig(ti.args))
 
     for prefixes, args, extra_commands, triple_in_cmd in run_list:
       # Execute non-filechecked runline.
       if not prefixes:
         print('NOTE: Executing non-FileChecked RUN line: ' + ' '.join(args), file=sys.stderr)
         exec_run_line(args)
         continue
 
if include_generated_funcs:
       common.dump_input_lines(output_lines, ti, prefix_set, '//')
 
       # Now generate all the checks.
       def check_generator(my_output_lines, prefixes, func):
         if '-emit-llvm' in clang_args:
           return common.add_ir_checks(my_output_lines, '//',
                                       prefixes,
                                       func_dict, func, False,
-                                      ti.args.function_signature,
+                                      common.check_func_sig(ti.args),
                                       ti.args.version,
                                       global_vars_seen_dict,
                                       is_filtered=builder.is_filtered())
         else:
           return asm.add_checks(my_output_lines, '//',
                                 prefixes,
                                 func_dict, func, global_vars_seen_dict,
                                 is_filtered=builder.is_filtered())
else:
               generated_prefixes.extend(
                   common.add_ir_checks(
                       output_lines,
                       '//',
                       filecheck_run_list,
                       func_dict,
                       mangled,
                       False,
-                      args.function_signature,
+                      common.check_func_sig(args),
                       args.version,
                       global_vars_seen_dict,
                       is_filtered=builder.is_filtered()))
               if line.rstrip('\n') == '//':
                 include_line = False
 
         if include_line:
           output_lines.append(line.rstrip('\n'))

llvm/utils/update_llc_test_checks.py

for ti in common.itertests(initial_args.tests, parser,
     else:
       check_indent = ''
 
     builder = common.FunctionTestBuilder(
         run_list=run_list,
         flags=type('', (object,), {
             'verbose': ti.args.verbose,
             'filters': ti.args.filters,
-            'function_signature': False,
             'check_attributes': False,
             'replace_value_regex': []}),
         scrubber_args=[ti.args],
-        path=ti.path)
+        path=ti.path,
+        record_args=False)
 
     for prefixes, llc_tool, llc_args, preprocess_cmd, triple_in_cmd, march_in_cmd in run_list:
       common.debug('Extracted LLC cmd:', llc_tool, llc_args)
       common.debug('Extracted FileCheck prefixes:', str(prefixes))
 
       raw_tool_output = common.invoke_tool(ti.args.llc_binary or llc_tool,
                                            llc_args, ti.path, preprocess_cmd,
                                            verbose=ti.args.verbose)

llvm/utils/update_test_checks.py

for l in ti.run_lines:
       # now, we just ignore all but the last.
       prefix_list.append((check_prefixes, tool_cmd_args, preprocess_cmd))
 
     global_vars_seen_dict = {}
     builder = common.FunctionTestBuilder(
       run_list=prefix_list,
       flags=ti.args,
       scrubber_args=[],
-      path=ti.path)
+      path=ti.path,
+      record_args=common.check_func_sig(ti.args))
 
     tool_binary = ti.args.tool_binary
     if not tool_binary:
       tool_binary = tool_basename
 
     for prefixes, tool_args, preprocess_cmd in prefix_list:
       common.debug('Extracted tool cmd: ' + tool_basename + ' ' + tool_args)
       common.debug('Extracted FileCheck prefixes: ' + str(prefixes))
if include_generated_funcs:
               output_lines, prefix_list, builder.func_order(), ';',
               lambda my_output_lines, prefixes, func: common.add_ir_checks(
                   my_output_lines,
                   ';',
                   prefixes,
                   func_dict,
                   func,
                   False,
-                  args.function_signature,
+                  common.check_func_sig(args),
                   args.version,
                   global_vars_seen_dict,
                   is_filtered=builder.is_filtered())))
     else:
       # "Normal" mode.
       for input_line_info in ti.iterlines(output_lines):
         input_line = input_line_info.line
         args = input_line_info.args
else:
           generated_prefixes.extend(
               common.add_ir_checks(
                   output_lines,
                   ';',
                   prefix_list,
                   func_dict,
                   func_name,
                   args.preserve_names,
-                  args.function_signature,
+                  common.check_func_sig(args),
                   args.version,
                   global_vars_seen_dict,
                   is_filtered=builder.is_filtered()))
           is_in_function_start = False
 
         m = common.IR_FUNCTION_RE.match(input_line)
         if m and not has_checked_pre_function_globals:
           if args.check_globals: