[tools] UpdateTestPrefix: improved verbosity

[mtrofin]

Summary

Warning by default only if a function ends up not having any matching
output for any prefixes - as well as unused prefixes (which should be
none/minimal noise)

[mtrofin]

newline

[spatel]

I've added a functionality question, but someone else should definitely look at the implementation because my python experience is minimal.

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll

5

If we remove "RV64I" here, then the last 2 functions would have no assertions, right? Do we want to issue a warning for that scenario? Right now, it seems to be silent on that case by default.

[mtrofin]

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll

5

No, the last 2 functions will have the RV32I assertions from the RUN line above.

[spatel]

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll

5

Ah, I see. But those functions would have no assertions for the 2nd RUN line. Is there a way to enable warnings for that case?

[mtrofin]

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll

5

That'd be outside the scope of this fix - this one just aims to provide the expected behavior that was sometimes working (because it was buggy) pre-e2dc306b1ac71258e6ce40a66e778527f282c355.

[mtrofin]

gentle reminder

[spatel]

I think this is an improvement from current behavior, so no objections from me, but hopefully someone else can take a look at the implementation.
@RKSimon might have some comments based on:
https://llvm.org/PR49189

[RKSimon]

In D96290#2563750, @spatel wrote:

I think this is an improvement from current behavior, so no objections from me, but hopefully someone else can take a look at the implementation.
@RKSimon might have some comments based on:
https://llvm.org/PR49189

I've replied on the bug , but we seem to have lost the original warning as part of D93078

[mtrofin]

In D96290#2563763, @RKSimon wrote:

In D96290#2563750, @spatel wrote:

I think this is an improvement from current behavior, so no objections from me, but hopefully someone else can take a look at the implementation.
@RKSimon might have some comments based on:
https://llvm.org/PR49189

I've replied on the bug , but we seem to have lost the original warning as part of D93078

As detailed on the bug, and on D93078, that warning was buggy. In particular, in the case of PR49189, it wouldn't have worked (tried by sync-ing before the patch and running the tool).

Are we confident this (i.e. warn if a RUN line's outputs aren't used) was the intended behavior? Perhaps it would be worth taking a step back and defining what the desired behavior of the tool is - independent from previous state of the code which, again, was buggy and untested (and, thus, can't serve as a specification).

[RKSimon]

It comes down to us wanting the update scripts to use lists of check-prefixes for every RUN line, allowing us to share prefixes at the function level, typically getting more and more specific down the list. That way we can minimise bloat in the check lines, but still have thorough coverage with many RUNs.

With the move to avoid unused prefixes, there's going to be a lot more cases where the prefix lists need editing every time a test is changed - we need the update scripts to tell us both when a prefix becomes newly unused, but also when a run suddenly isn't checked at all.

[mtrofin]

In D96290#2563855, @RKSimon wrote:

It comes down to us wanting the update scripts to use lists of check-prefixes for every RUN line, allowing us to share prefixes at the function level, typically getting more and more specific down the list. That way we can minimise bloat in the check lines, but still have thorough coverage with many RUNs.

With the move to avoid unused prefixes, there's going to be a lot more cases where the prefix lists need editing every time a test is changed - we need the update scripts to tell us both when a prefix becomes newly unused, but also when a run suddenly isn't checked at all.

I'm confused - IIRC (https://lists.llvm.org/pipermail/llvm-dev/2021-February/148309.html), the scenario for update_[llc]_test_checks was better nuanced, and it seemed like the position was that a) those tests could, indeed, validly feature unused prefixes, and b) the tools (update_[]_test_checks) would potentially handle that, or the tests would explicitly opt in.

This patch was about addressing overly-verbose messages - https://lists.llvm.org/pipermail/llvm-dev/2021-February/148272.html

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_nowarn.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefixes=CHECK,RV32I
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefixes=CHECK,RV64I
+
+define i32 @addi(i32 %a) nounwind {
+  %1 = add i32 %a, 1
+  ret i32 %1
+}
+
+define i32 @slti(i32 %a) nounwind {
+  %1 = icmp slt i32 %a, 2
+  %2 = zext i1 %1 to i32
+  ret i32 %2
+}
+
+define i32 @sltiu(i32 %a) nounwind {
+  %1 = icmp ult i32 %a, 3
+  %2 = zext i1 %1 to i32
+  ret i32 %2
+}

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/prefix_warning_verbosity_warn.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=CHECK
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=CHECK
+
+define i32 @addi(i32 %a) nounwind {
+  %1 = add i32 %a, 1
+  ret i32 %1
+}
+
+define i32 @slti(i32 %a) nounwind {
+  %1 = icmp slt i32 %a, 2
+  %2 = zext i1 %1 to i32
+  ret i32 %2
+}
+
+define i32 @sltiu(i32 %a) nounwind {
+  %1 = icmp ult i32 %a, 3
+  %2 = zext i1 %1 to i32
+  ret i32 %2
+}

llvm/test/tools/UpdateTestChecks/update_llc_test_checks/prefix_warning_verbosity.test

+# REQUIRES: riscv-registered-target
+
+# The 2 files below are identical insofar as IR, they differ in the assert prefixes.
+# The _nowarn case generates assertions for all 3 functions. The _warn case
+# ends up with conflicting output for 2 of the functions, thus producing a
+# warning for each.
+#
+# RUN: cp -f %S/Inputs/prefix_warning_verbosity_nowarn.ll %t_nowarn.ll
+# RUN: cp -f %S/Inputs/prefix_warning_verbosity_warn.ll %t_warn.ll
+
+# RUN: %update_llc_test_checks %t_nowarn.ll 2>&1 | FileCheck %s --check-prefix=NOWARN --allow-empty
+# RUN: %update_llc_test_checks %t_warn.ll 2>&1  | FileCheck %s --check-prefix=WARN
+
+# NOWARN-NOT: WARNING
+# WARN: WARNING: Function slti has no asserts due to conflicts
+# WARN: WARNING: Function sltiu has no asserts due to conflicts

llvm/utils/UpdateTestChecks/common.py

 else:
   import string
 
 ##### Common utilities for update_*test_checks.py
 
 
 _verbose = False
 
+PF_DISABLED = 0
+PF_UNUSED = 1
+PF_FCT_NO_ASSERTS = 2
+PF_MAX_VERBOSITY = 3
+
 def parse_commandline_args(parser):
   parser.add_argument('--include-generated-funcs', action='store_true',
                       help='Output checks for functions not in source')
   parser.add_argument('-v', '--verbose', action='store_true',
                       help='Show verbose output')
   parser.add_argument('-u', '--update-only', action='store_true',
                       help='Only update test if it was already autogened')
   parser.add_argument('--force-update', action='store_true',
                       help='Update test even if it was autogened by a different script')
   parser.add_argument('--enable', action='store_true', dest='enabled', default=True,
                        help='Activate CHECK line generation from this point forward')
   parser.add_argument('--disable', action='store_false', dest='enabled',
                       help='Deactivate CHECK line generation from this point forward')
+  parser.add_argument('--prefix-warnings-verbosity', action='store',
+                      default=PF_FCT_NO_ASSERTS,
+                      type=int,
+                      dest='prefix_warnings_verbosity',
+                      help='0: Disabled\n'
+                      '1: Warn only on unused prefixes.\n'
+                      '2: Warn if a function has no assertions.\n'
+                      '3: Warn if a function assertions are lost due to a conflicting RUN line')
+
   args = parser.parse_args()
-  global _verbose
+  global _verbose, _prefix_warnings_verbosity
   _verbose = args.verbose
+  _prefix_warnings_verbosity = args.prefix_warnings_verbosity
   return args
 
 
 class InputLineInfo(object):
   def __init__(self, line, line_number, args, argv):
     self.line = line
     self.line_number = line_number
     self.args = args

 class FunctionTestBuilder:
   def __init__(self, run_list, flags, scrubber_args):
     self._verbose = flags.verbose
     self._record_args = flags.function_signature
     self._check_attributes = flags.check_attributes
     self._scrubber_args = scrubber_args
     self._func_dict = {}
     self._func_order = {}
+    self._per_fct_prefix_count = {}
     for tuple in run_list:
       for prefix in tuple[0]:
         self._func_dict.update({prefix:dict()})
         self._func_order.update({prefix: []})
 
   def finish_and_get_func_dict(self):
+    if _prefix_warnings_verbosity <= PF_DISABLED:
+      return self._func_dict
+    if _prefix_warnings_verbosity >= PF_FCT_NO_ASSERTS:
+      for fct in self._per_fct_prefix_count:
+        if self._per_fct_prefix_count[fct] == 0:
+          warn('Function %s has no asserts due to conflicts from different RUN lines' % fct)
+
     for prefix in self._get_failed_prefixes():
       warn('Prefix %s had conflicting output from different RUN lines for all functions' % (prefix,))
     return self._func_dict
 
   def func_order(self):
     return self._func_order
-  
   def process_run_line(self, function_re, scrubber, raw_tool_output, prefixes):
     for m in function_re.finditer(raw_tool_output):
       if not m:
         continue
       func = m.group('func')
       body = m.group('body')
       attrs = m.group('attrs') if self._check_attributes else ''
       # Determine if we print arguments, the opening brace, or nothing after the
for m in function_re.finditer(raw_tool_output):
               self._func_dict[prefix][func].args_and_sig = args_and_sig
               continue
             else:
               # This means a previous RUN line produced a body for this function
               # that is different from the one produced by this current RUN line,
               # so the body can't be common accross RUN lines. We use None to
               # indicate that.
               self._func_dict[prefix][func] = None
+              assert func in self._per_fct_prefix_count and self._per_fct_prefix_count[func] > 0
+              self._per_fct_prefix_count[func] -= 1
+              if _prefix_warnings_verbosity >= PF_MAX_VERBOSITY:
+                warn('Function %s had conflicting output from different RUN lines for prefix %s' % (
+                    func, prefix))
               continue
 
         self._func_dict[prefix][func] = function_body(
             scrubbed_body, scrubbed_extra, args_and_sig, attrs)
         self._func_order[prefix].append(func)
+        if func not in self._per_fct_prefix_count:
+          self._per_fct_prefix_count[func] = 0
+        self._per_fct_prefix_count[func] += 1
 
   def _get_failed_prefixes(self):
     # This returns the list of those prefixes that failed to match any function,
     # because there were conflicting bodies produced by different RUN lines, in
     # all instances of the prefix. Effectively, this prefix is unused and should
     # be removed.
     for prefix in self._func_dict:
-      if (self._func_dict[prefix] and 
+      if (self._func_dict[prefix] and
           (not [fct for fct in self._func_dict[prefix]
                 if self._func_dict[prefix][fct] is not None])):
         yield prefix
 
-
 ##### Generator of LLVM IR CHECK lines
 
 SCRUB_IR_COMMENT_RE = re.compile(r'\s*;.*')
 
 # TODO: We should also derive check lines for global, debug, loop declarations, etc..
 
 class NamelessValue:
     def __init__(self, check_prefix, ir_prefix, ir_regexp):