[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

255

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 ↗

(On Diff #501810)

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

255

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.funcsig.v2.expected

-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --function-signature --version 2
+// 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

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: --function-signature --version 2
+// 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

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 parse_args(parser, argv):
+  args = parser.parse_args(argv)
+  if args.version >= 2:
+    args.function_signature = True
+  return args
 
 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:])
+      args = parse_args(parser, 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:
for action in parser._actions:
     value = getattr(args, action.dest)
     if action.const is not None:  # action stores a constant (usually True/False)
       # Skip actions with different constant values (this happens with boolean
       # --foo/--no-foo options)
       if value != action.const:
         continue
     if parser.get_default(action.dest) == value:
       continue  # Don't add default values
+    if action.dest == 'function_signature' and args.version >= 2:
+      continue # Enabled by default in version 2
     if action.dest == 'filters':
       # Create a separate option for each filter element.  The value is a list
       # of Filter objects.
       for elem in value:
         opt_name = 'filter-out' if elem.is_filter_out else 'filter'
         opt_value = elem.pattern()
         new_arg = '--%s "%s" ' % (opt_name, opt_value.strip('"'))
         if new_arg not in autogenerated_note_args:

 
 
 def check_for_command(line, parser, args, argv, argparse_callback):
   cmd_m = UTC_ARGS_CMD.match(line)
   if cmd_m:
     for option in shlex.split(cmd_m.group('cmd').strip()):
       if option:
         argv.append(option)
-    args = parser.parse_args(filter(lambda arg: arg not in args.tests, argv))
+    args = parse_args(parser, filter(lambda arg: arg not in args.tests, argv))
     if argparse_callback is not None:
       argparse_callback(args)
   return args, argv
 
 def find_arg_in_test(test_info, get_arg_to_check, arg_string, is_global):
   result = get_arg_to_check(test_info.args)
   if not result and is_global:
     # See if this has been specified via UTC_ARGS.  This is a "global" option