[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 537299.
hnrklssn added a comment.

Update test cases after rebasing on ToT


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.all.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.all.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/generated-funcs.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.noglobals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.transitiveglobals.expected
  llvm/test/tools/UpdateTestChecks/update_test_checks/various_ir_values.test
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py
  llvm/utils/update_test_checks.py

Index: llvm/utils/update_test_checks.py
===
--- llvm/utils/update_test_checks.py
+++ llvm/utils/update_test_checks.py
@@ -79,7 +79,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -195,7 +198,7 @@
 common.dump_input_lines(output_lines, ti, prefix_set, ";")
 
 args = ti.args
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -205,6 +208,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 
@@ -272,6 +276,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 has_checked_pre_function_globals = True
@@ -301,7 +306,7 @@
 continue
 is_in_function = is_in_function_start = True
 
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -311,6 +316,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 False,
+args.check_globals,
 )
 )
 if ti.args.gen_unused_prefix_body:
Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -205,7 +205,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -436,7 +439,7 @@
 is_filtered=builder.is_filtered(),
 )
 
-if ti.args.check_globals:
+if ti.args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -444,8 +447,9 @@
 run_list,
 output_lines,
   

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

(The patch description could use an update...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 537286.
hnrklssn added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: jplehr, sstefan1.

Add unnamed global test case

Added cases in generated-funcs.c with --check-globals all, testing that named 
and unnamed globals are matched differently.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.all.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.all.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/generated-funcs.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.noglobals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.transitiveglobals.expected
  llvm/test/tools/UpdateTestChecks/update_test_checks/various_ir_values.test
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py
  llvm/utils/update_test_checks.py

Index: llvm/utils/update_test_checks.py
===
--- llvm/utils/update_test_checks.py
+++ llvm/utils/update_test_checks.py
@@ -79,7 +79,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -195,7 +198,7 @@
 common.dump_input_lines(output_lines, ti, prefix_set, ";")
 
 args = ti.args
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -205,6 +208,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 
@@ -272,6 +276,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 has_checked_pre_function_globals = True
@@ -301,7 +306,7 @@
 continue
 is_in_function = is_in_function_start = True
 
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -311,6 +316,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 False,
+args.check_globals,
 )
 )
 if ti.args.gen_unused_prefix_body:
Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -205,7 +205,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -436,7 +439,7 @@
 is_filtered=builder.is_filtered(),
 )
 
-if ti.args.check_globals:
+if ti.args.check_globals != 'none':
 generated_prefixes.extend(
 c

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: StephenFan.

LGTM

> Fixed. Does this test case exist somewhere? I couldn't find it upstream.

That wasn't an existing test case. Though if you add a `--check-globals all` 
variant to generated-funcs.c, I believe that would cover that case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added a comment.

@nikic Are you happy with the current patchset?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added a comment.

@nikic Ping :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added a comment.

In D148216#4434171 , @nikic wrote:

> There are some test failures.
>
> I believe there is one bug with the handling of unnamed globals. Previously 
> we produced this:
>
>   ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py 
> UTC_ARGS: --check-globals --version 2
>   ; RUN: opt -S < %s | FileCheck %s
>   @0 = global i32 0
>   
>   ;.
>   ; CHECK: @[[GLOB0:[0-9]+]] = global i32 0
>   ;.
>   define i32 @test() {
>   ; CHECK-LABEL: define i32 @test() {
>   ; CHECK-NEXT:[[V:%.*]] = load i32, ptr @[[GLOB0]], align 4
>   ; CHECK-NEXT:ret i32 [[V]]
>   ;
> %v = load i32, ptr @0
> ret i32 %v
>   }
>
> And now we instead check `CHECK: @0 = global i32 0`, so there is a mismatch 
> between definition and use. I believe it does make sense to keep the wildcard 
> names for unnamed globals.
>
> On the same test case, if I keep rerunning update_test_checks on the same 
> file, it keeps adding extra `CHECK: @0 = global i32 0` lines at the start 
> (before the first `;.`. That didn't happen previously.

Fixed. Does this test case exist somewhere? I couldn't find it upstream.




Comment at: 
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected:9
 define ptr @foo(ptr %s) nounwind uwtable readnone optsize ssp {
-; CHECK: Function Attrs: mustprogress nofree norecurse nosync nounwind optsize 
ssp willreturn memory(none) uwtable
+; CHECK: Function Attrs: nofree norecurse nosync nounwind optsize ssp 
willreturn memory(none) uwtable
 ; CHECK-LABEL: define {{[^@]+}}@foo

nikic wrote:
> Huh, where does this change come from?
I thought it was an existing test failure upstream, but it seems to have 
disappeared after doing a full rebuild clang+llvm.



Comment at: llvm/utils/UpdateTestChecks/common.py:991
+NamelessValue(r"META"   , "!" , r"![a-z.]+ "   , r"![0-9]+"
 , None ) ,
+]
+

nikic wrote:
> Does this still comply with the formatter?
No, I guess maintaining this structure isn't worth it long term with the 
autoformatter


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 532868.
hnrklssn marked 4 inline comments as done.
hnrklssn added a comment.

Added back regex matchers for anonymous global values


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/generated-funcs.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.noglobals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.transitiveglobals.expected
  llvm/test/tools/UpdateTestChecks/update_test_checks/various_ir_values.test
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py
  llvm/utils/update_test_checks.py

Index: llvm/utils/update_test_checks.py
===
--- llvm/utils/update_test_checks.py
+++ llvm/utils/update_test_checks.py
@@ -79,7 +79,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -195,7 +198,7 @@
 common.dump_input_lines(output_lines, ti, prefix_set, ";")
 
 args = ti.args
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -205,6 +208,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 
@@ -272,6 +276,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 has_checked_pre_function_globals = True
@@ -301,7 +306,7 @@
 continue
 is_in_function = is_in_function_start = True
 
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -311,6 +316,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 False,
+args.check_globals,
 )
 )
 if ti.args.gen_unused_prefix_body:
Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -205,7 +205,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -436,7 +439,7 @@
 is_filtered=builder.is_filtered(),
 )
 
-if ti.args.check_globals:
+if ti.args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -444,8 +447,9 @@
 run_list,
 output_lines,
 global_vars_seen_dict,
+False,
 True,
-True,
+  

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

There are some test failures.

I believe there is one bug with the handling of unnamed globals. Previously we 
produced this:

  ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py 
UTC_ARGS: --check-globals --version 2
  ; RUN: opt -S < %s | FileCheck %s
  @0 = global i32 0
  
  ;.
  ; CHECK: @[[GLOB0:[0-9]+]] = global i32 0
  ;.
  define i32 @test() {
  ; CHECK-LABEL: define i32 @test() {
  ; CHECK-NEXT:[[V:%.*]] = load i32, ptr @[[GLOB0]], align 4
  ; CHECK-NEXT:ret i32 [[V]]
  ;
%v = load i32, ptr @0
ret i32 %v
  }

And now we instead check `CHECK: @0 = global i32 0`, so there is a mismatch 
between definition and use. I believe it does make sense to keep the wildcard 
names for unnamed globals.

On the same test case, if I keep rerunning update_test_checks on the same file, 
it keeps adding extra `CHECK: @0 = global i32 0` lines at the start (before the 
first `;.`. That didn't happen previously.




Comment at: clang/test/utils/update_cc_test_checks/Inputs/annotations.c:1
+// UTC_ARGS: --version 3
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks 
-ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s

It would be more typical to pass `--version 3` when calling 
update_cc_test_checks. (Note that this is only a quirk of the test setup. 
Outside tests the `--version 3` is implied for new tests.)



Comment at: 
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected:9
 define ptr @foo(ptr %s) nounwind uwtable readnone optsize ssp {
-; CHECK: Function Attrs: mustprogress nofree norecurse nosync nounwind optsize 
ssp willreturn memory(none) uwtable
+; CHECK: Function Attrs: nofree norecurse nosync nounwind optsize ssp 
willreturn memory(none) uwtable
 ; CHECK-LABEL: define {{[^@]+}}@foo

Huh, where does this change come from?



Comment at: llvm/utils/UpdateTestChecks/common.py:991
+NamelessValue(r"META"   , "!" , r"![a-z.]+ "   , r"![0-9]+"
 , None ) ,
+]
+

Does this still comply with the formatter?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn marked an inline comment as done.
hnrklssn added inline comments.



Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

nikic wrote:
> nikic wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > nikic wrote:
> > > > > We should also not print the `all` argument for `--check-globals` 
> > > > > argument for `version < 3`, otherwise that will introduce a spurious 
> > > > > change in all such tests.
> > > > I don't know how to dynamically change the `--check-globals` option 
> > > > between `store_true` and `choices`, since the behaviour can itself be 
> > > > affected by which argument is passed to the `--version` option. So the 
> > > > way I see it there's no way of knowing how to parse between two 
> > > > different option types ahead of time. For the default when no option is 
> > > > specified the parsing is the same however, so we can simply infer later 
> > > > what option is implied based on the version.
> > > It's not necessary to change the option, just how it is printed. I.e. add 
> > > something like this:
> > > ```
> > > if args.version < 3 and value == "all":
> > > # Don't include argument value in older versions.
> > > autogenerated_note_args += "--check-globals "
> > > continue
> > > ```
> > Ah, I get what you're saying now -- old UTC_ARGS are not accepted at all 
> > anymore, and trying to update existing tests will just fail with an error!
> > 
> > We do need old UTC_ARGS to work. I think you can make the argument value 
> > optional using these options:
> > ```
> > nargs="?", 
> > const="default",
> > default="default",
> > choices=["none", "smart", "all", "default"],
> > ```
> > 
> > Or possibly omit const/default and handle None instead.
> Correction, I think the right options are:
> ```
> nargs="?",
> const="all",
> default="default",
> choices=["none", "smart", "all"],
> ```
> That is, use `"default"` is it's not specified at all, use `"all"` if only 
> `--check-globals` is passed and also accept `--check-globals none|smart|all`.
Ah nice, I did not expect the library to support this use case!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 532719.
hnrklssn added a comment.

Keep supporting --check-globals without explicit level as meaning 
'--check-globals all'


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/generated-funcs.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.noglobals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.transitiveglobals.expected
  llvm/test/tools/UpdateTestChecks/update_test_checks/various_ir_values.test
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py
  llvm/utils/update_test_checks.py

Index: llvm/utils/update_test_checks.py
===
--- llvm/utils/update_test_checks.py
+++ llvm/utils/update_test_checks.py
@@ -79,7 +79,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -195,7 +198,7 @@
 common.dump_input_lines(output_lines, ti, prefix_set, ";")
 
 args = ti.args
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -205,6 +208,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 
@@ -272,6 +276,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 has_checked_pre_function_globals = True
@@ -301,7 +306,7 @@
 continue
 is_in_function = is_in_function_start = True
 
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -311,6 +316,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 False,
+args.check_globals,
 )
 )
 if ti.args.gen_unused_prefix_body:
Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -205,7 +205,10 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+nargs="?",
+const="all",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -436,7 +439,7 @@
 is_filtered=builder.is_filtered(),
 )
 
-if ti.args.check_globals:
+if ti.args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -444,8 +447,9 @@
 run_list,
 output_lines,
 global_vars_seen_dict,
+

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

nikic wrote:
> nikic wrote:
> > hnrklssn wrote:
> > > nikic wrote:
> > > > We should also not print the `all` argument for `--check-globals` 
> > > > argument for `version < 3`, otherwise that will introduce a spurious 
> > > > change in all such tests.
> > > I don't know how to dynamically change the `--check-globals` option 
> > > between `store_true` and `choices`, since the behaviour can itself be 
> > > affected by which argument is passed to the `--version` option. So the 
> > > way I see it there's no way of knowing how to parse between two different 
> > > option types ahead of time. For the default when no option is specified 
> > > the parsing is the same however, so we can simply infer later what option 
> > > is implied based on the version.
> > It's not necessary to change the option, just how it is printed. I.e. add 
> > something like this:
> > ```
> > if args.version < 3 and value == "all":
> > # Don't include argument value in older versions.
> > autogenerated_note_args += "--check-globals "
> > continue
> > ```
> Ah, I get what you're saying now -- old UTC_ARGS are not accepted at all 
> anymore, and trying to update existing tests will just fail with an error!
> 
> We do need old UTC_ARGS to work. I think you can make the argument value 
> optional using these options:
> ```
> nargs="?", 
> const="default",
> default="default",
> choices=["none", "smart", "all", "default"],
> ```
> 
> Or possibly omit const/default and handle None instead.
Correction, I think the right options are:
```
nargs="?",
const="all",
default="default",
choices=["none", "smart", "all"],
```
That is, use `"default"` is it's not specified at all, use `"all"` if only 
`--check-globals` is passed and also accept `--check-globals none|smart|all`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

nikic wrote:
> hnrklssn wrote:
> > nikic wrote:
> > > We should also not print the `all` argument for `--check-globals` 
> > > argument for `version < 3`, otherwise that will introduce a spurious 
> > > change in all such tests.
> > I don't know how to dynamically change the `--check-globals` option between 
> > `store_true` and `choices`, since the behaviour can itself be affected by 
> > which argument is passed to the `--version` option. So the way I see it 
> > there's no way of knowing how to parse between two different option types 
> > ahead of time. For the default when no option is specified the parsing is 
> > the same however, so we can simply infer later what option is implied based 
> > on the version.
> It's not necessary to change the option, just how it is printed. I.e. add 
> something like this:
> ```
> if args.version < 3 and value == "all":
> # Don't include argument value in older versions.
> autogenerated_note_args += "--check-globals "
> continue
> ```
Ah, I get what you're saying now -- old UTC_ARGS are not accepted at all 
anymore, and trying to update existing tests will just fail with an error!

We do need old UTC_ARGS to work. I think you can make the argument value 
optional using these options:
```
nargs="?", 
const="default",
default="default",
choices=["none", "smart", "all", "default"],
```

Or possibly omit const/default and handle None instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

hnrklssn wrote:
> nikic wrote:
> > We should also not print the `all` argument for `--check-globals` argument 
> > for `version < 3`, otherwise that will introduce a spurious change in all 
> > such tests.
> I don't know how to dynamically change the `--check-globals` option between 
> `store_true` and `choices`, since the behaviour can itself be affected by 
> which argument is passed to the `--version` option. So the way I see it 
> there's no way of knowing how to parse between two different option types 
> ahead of time. For the default when no option is specified the parsing is the 
> same however, so we can simply infer later what option is implied based on 
> the version.
It's not necessary to change the option, just how it is printed. I.e. add 
something like this:
```
if args.version < 3 and value == "all":
# Don't include argument value in older versions.
autogenerated_note_args += "--check-globals "
continue
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added a comment.

In D148216#4431456 , @jdoerfert wrote:

> could you put a little more information in the commit message please. "It 
> won't do X when we do Y", could mean a lot of things. We don't do Y anymore, 
> or we do X' now, with various choices for X'.

I've updated the commit message to clarify what it means to not emit hardcoded 
identifiers, and that we instead emit regex checkers. Was that what you were 
aiming for, or are there other parts that you would like me to clarify?




Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

nikic wrote:
> We should also not print the `all` argument for `--check-globals` argument 
> for `version < 3`, otherwise that will introduce a spurious change in all 
> such tests.
I don't know how to dynamically change the `--check-globals` option between 
`store_true` and `choices`, since the behaviour can itself be affected by which 
argument is passed to the `--version` option. So the way I see it there's no 
way of knowing how to parse between two different option types ahead of time. 
For the default when no option is specified the parsing is the same however, so 
we can simply infer later what option is implied based on the version.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 532649.
hnrklssn added a comment.

Rebase after reformatting with Black, update commit message


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/generated-funcs.test
  clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
  clang/test/utils/update_cc_test_checks/global-value-regex.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.noglobals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.transitiveglobals.expected
  llvm/test/tools/UpdateTestChecks/update_test_checks/generated_funcs.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/generated_funcs_prefix_reuse.test
  llvm/test/tools/UpdateTestChecks/update_test_checks/various_ir_values.test
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py
  llvm/utils/update_test_checks.py

Index: llvm/utils/update_test_checks.py
===
--- llvm/utils/update_test_checks.py
+++ llvm/utils/update_test_checks.py
@@ -79,7 +79,8 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -195,7 +196,7 @@
 common.dump_input_lines(output_lines, ti, prefix_set, ";")
 
 args = ti.args
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -205,6 +206,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 
@@ -272,6 +274,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 True,
+args.check_globals,
 )
 )
 has_checked_pre_function_globals = True
@@ -301,7 +304,7 @@
 continue
 is_in_function = is_in_function_start = True
 
-if args.check_globals:
+if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(
 builder.global_var_dict(),
@@ -311,6 +314,7 @@
 global_vars_seen_dict,
 args.preserve_names,
 False,
+args.check_globals,
 )
 )
 if ti.args.gen_unused_prefix_body:
Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -205,7 +205,8 @@
 )
 parser.add_argument(
 "--check-globals",
-action="store_true",
+default="default",
+choices=["none", "smart", "all"],
 help="Check global entries (global variables, metadata, attribute sets, ...) for functions",
 )
 parser.add_argument("tests", nargs="+")
@@ -436,7 +437,7 @@
 is_filtered=builder.is_filtered(),
 )
 
-if ti.args.check_globals:
+  

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

could you put a little more information in the commit message please. "It won't 
do X when we do Y", could mean a lot of things. We don't do Y anymore, or we do 
X' now, with various choices for X'.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Could you please rebase the patch to current main? I wasn't able to apply it.

The behavior you describe sounds reasonable to me.




Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

We should also not print the `all` argument for `--check-globals` argument for 
`version < 3`, otherwise that will introduce a spurious change in all such 
tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added a comment.

@nikic Ping :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn marked an inline comment as done.
hnrklssn added a comment.

@nikic Did you have a look at the new patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn marked an inline comment as done.
hnrklssn added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

nikic wrote:
> hnrklssn wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > nikic wrote:
> > > > > nikic wrote:
> > > > > > hnrklssn wrote:
> > > > > > > hnrklssn wrote:
> > > > > > > > nikic wrote:
> > > > > > > > > hnrklssn wrote:
> > > > > > > > > > delcypher wrote:
> > > > > > > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what 
> > > > > > > > > > > `META2` actually refers to. Should we?
> > > > > > > > > > > 
> > > > > > > > > > > Not something that has to be fixed in this patch, more 
> > > > > > > > > > > just an observation.
> > > > > > > > > > Indeed this is true for metadata in general, presumably 
> > > > > > > > > > because the RHS often refer to things like other metadata 
> > > > > > > > > > identifiers. In the case of annotations they seem to always 
> > > > > > > > > > refer to simple strings however, so it would be feasible to 
> > > > > > > > > > do a straight match without having to do recursive matching 
> > > > > > > > > > or complex regexes to determine which part of the metadata 
> > > > > > > > > > to match against.
> > > > > > > > > > 
> > > > > > > > > > In many cases with metadata attached to IR nodes, multiple 
> > > > > > > > > > nodes refer to the same metadata node, so at least you 
> > > > > > > > > > verify that they still are consistent. But I agree that 
> > > > > > > > > > verifying the content would be a great future addition.
> > > > > > > > > You need to pass `--check-globals` to check the actual 
> > > > > > > > > metadata.
> > > > > > > > When I add that to this test case it adds
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > //.
> > > > > > > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > > > > > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > > > > > > "stack-protector-buffer-size"="8" 
> > > > > > > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > > > > > > //.
> > > > > > > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > > > > > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > > > > > > (g...@github.com:llvm/llvm-project.git 
> > > > > > > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > > > > > > // CHECK: !2 = !{!"auto-init"}
> > > > > > > > //.
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > So it seems to just be doing a simple literal matching on all 
> > > > > > > > metadata, regardless of whether we captured that metadata in 
> > > > > > > > any filecheck variable. And it still doesn't use the META2 
> > > > > > > > variable to match the definition. Am I missing something? If we 
> > > > > > > > use the literal metadata names instead of variable matching for 
> > > > > > > > the definitions, there isn't much point in doing variable 
> > > > > > > > matching for the metadata uses either, since the test still 
> > > > > > > > very much relies on the metadata numbering being stable.
> > > > > > > @nikic Do you have more information to add about how metadata 
> > > > > > > definition matchers can be generated without hardcoding 
> > > > > > > everything (which is kind of the opposite of what this patch is 
> > > > > > > trying to do), or in general if you're happy with the state of 
> > > > > > > the PR?
> > > > > > This works fine with update_test_checks, so it must be some bug in 
> > > > > > update_cc_test_checks in particular. From a quick look, I suspect 
> > > > > > it's because 
> > > > > > https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
> > > > > >  hardcodes a True value, while update_test_checks makes this 
> > > > > > dependent on `--preserve-names`, which is disabled by default there.
> > > > > Or maybe just test this with update_test_checks? This change is 
> > > > > valuable for that script as well, and it doesn't have this extra 
> > > > > issue.
> > > > I couldn't find any uses of 
> > > > `--preserve-names` in the entire repo (not even in tests for 
> > > > update_test_checks), so I went ahead and changed update_cc_test_checks 
> > > > to pass True instead.
> > > > 
> > > > While at it I added 2 new options to match some, but not necessarily 
> > > > all, globals. I set the least verbose one (other than "none") as the 
> > > > default, emitting checks for the definitions of all globals that we 
> > > > emit matches for in the IR. Do you agree that this is reasonable?
> > > That sounds like a nice approach. My main question would be whether 
> > > `--check-globals seen` is the right default: My gut feeling here is that 
> > > this will introduce additional checks for attributes/metadata in tests 
> > > that don't actually care about it, but I'm not particularly sure about 
> > > that.
> > > 
> > > Y

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 526606.
hnrklssn added a comment.

Revert checker behaviour for global variables to check hard-coded identifiers
Remove 'seen' mode
Replace 'transitive' mode with 'smart', where attributes are no longer checked 
as metadata
Introduce version bump to v3, and make 'smart' mode the default in v3


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/generated-funcs.test
  clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
  clang/test/utils/update_cc_test_checks/global-value-regex.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.noglobals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.transitiveglobals.expected
  llvm/test/tools/UpdateTestChecks/update_test_checks/generated_funcs.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/generated_funcs_prefix_reuse.test
  llvm/test/tools/UpdateTestChecks/update_test_checks/various_ir_values.test
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py
  llvm/utils/update_test_checks.py

Index: llvm/utils/update_test_checks.py
===
--- llvm/utils/update_test_checks.py
+++ llvm/utils/update_test_checks.py
@@ -59,7 +59,7 @@
   help='Remove attribute annotations (#0) from the end of check line')
   parser.add_argument('--check-attributes', action='store_true',
   help='Check "Function Attributes" for functions')
-  parser.add_argument('--check-globals', action='store_true',
+  parser.add_argument('--check-globals', default='default', choices=['none', 'smart', 'all'],
   help='Check global entries (global variables, metadata, attribute sets, ...) for functions')
   parser.add_argument('tests', nargs='+')
   initial_args = common.parse_commandline_args(parser)
@@ -158,12 +158,12 @@
   common.dump_input_lines(output_lines, ti, prefix_set, ';')
 
   args = ti.args
-  if args.check_globals:
+  if args.check_globals != 'none':
 generated_prefixes.extend(
 common.add_global_checks(builder.global_var_dict(), ';',
  prefix_list, output_lines,
  global_vars_seen_dict, args.preserve_names,
- True))
+ True, args.check_globals))
 
   # Now generate all the checks.
   generated_prefixes.extend(
@@ -211,12 +211,11 @@
 
 m = common.IR_FUNCTION_RE.match(input_line)
 if m and not has_checked_pre_function_globals:
-  if args.check_globals:
-generated_prefixes.extend(
-common.add_global_checks(builder.global_var_dict(), ';',
- prefix_list, output_lines,
- global_vars_seen_dict,
- args.preserve_names, True))
+  generated_prefixes.extend(
+  common.add_global_checks(builder.global_var_dict(), ';',
+   prefix_list, output_lines,
+   global_vars_seen_dict,
+   args.preserve_names, True, args.check_globals))
   has_checked_pre_function_globals = True
 
 if common.should_add_line_to_output(input_line, prefix_set, not is_in_function):
@@ -240,11 +239,11 @@
   continue
 is_in_function = is_in_function_start = True
 
-if args.check_globals:
+if args.check_globals != 'none':
   generated_prefixes.extend(

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> nikic wrote:
> > hnrklssn wrote:
> > > nikic wrote:
> > > > nikic wrote:
> > > > > hnrklssn wrote:
> > > > > > hnrklssn wrote:
> > > > > > > nikic wrote:
> > > > > > > > hnrklssn wrote:
> > > > > > > > > delcypher wrote:
> > > > > > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what 
> > > > > > > > > > `META2` actually refers to. Should we?
> > > > > > > > > > 
> > > > > > > > > > Not something that has to be fixed in this patch, more just 
> > > > > > > > > > an observation.
> > > > > > > > > Indeed this is true for metadata in general, presumably 
> > > > > > > > > because the RHS often refer to things like other metadata 
> > > > > > > > > identifiers. In the case of annotations they seem to always 
> > > > > > > > > refer to simple strings however, so it would be feasible to 
> > > > > > > > > do a straight match without having to do recursive matching 
> > > > > > > > > or complex regexes to determine which part of the metadata to 
> > > > > > > > > match against.
> > > > > > > > > 
> > > > > > > > > In many cases with metadata attached to IR nodes, multiple 
> > > > > > > > > nodes refer to the same metadata node, so at least you verify 
> > > > > > > > > that they still are consistent. But I agree that verifying 
> > > > > > > > > the content would be a great future addition.
> > > > > > > > You need to pass `--check-globals` to check the actual metadata.
> > > > > > > When I add that to this test case it adds
> > > > > > > 
> > > > > > > ```
> > > > > > > //.
> > > > > > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > > > > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > > > > > "stack-protector-buffer-size"="8" 
> > > > > > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > > > > > //.
> > > > > > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > > > > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > > > > > (g...@github.com:llvm/llvm-project.git 
> > > > > > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > > > > > // CHECK: !2 = !{!"auto-init"}
> > > > > > > //.
> > > > > > > ```
> > > > > > > 
> > > > > > > So it seems to just be doing a simple literal matching on all 
> > > > > > > metadata, regardless of whether we captured that metadata in any 
> > > > > > > filecheck variable. And it still doesn't use the META2 variable 
> > > > > > > to match the definition. Am I missing something? If we use the 
> > > > > > > literal metadata names instead of variable matching for the 
> > > > > > > definitions, there isn't much point in doing variable matching 
> > > > > > > for the metadata uses either, since the test still very much 
> > > > > > > relies on the metadata numbering being stable.
> > > > > > @nikic Do you have more information to add about how metadata 
> > > > > > definition matchers can be generated without hardcoding everything 
> > > > > > (which is kind of the opposite of what this patch is trying to do), 
> > > > > > or in general if you're happy with the state of the PR?
> > > > > This works fine with update_test_checks, so it must be some bug in 
> > > > > update_cc_test_checks in particular. From a quick look, I suspect 
> > > > > it's because 
> > > > > https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
> > > > >  hardcodes a True value, while update_test_checks makes this 
> > > > > dependent on `--preserve-names`, which is disabled by default there.
> > > > Or maybe just test this with update_test_checks? This change is 
> > > > valuable for that script as well, and it doesn't have this extra issue.
> > > I couldn't find any uses of 
> > > `--preserve-names` in the entire repo (not even in tests for 
> > > update_test_checks), so I went ahead and changed update_cc_test_checks to 
> > > pass True instead.
> > > 
> > > While at it I added 2 new options to match some, but not necessarily all, 
> > > globals. I set the least verbose one (other than "none") as the default, 
> > > emitting checks for the definitions of all globals that we emit matches 
> > > for in the IR. Do you agree that this is reasonable?
> > That sounds like a nice approach. My main question would be whether 
> > `--check-globals seen` is the right default: My gut feeling here is that 
> > this will introduce additional checks for attributes/metadata in tests that 
> > don't actually care about it, but I'm not particularly sure about that.
> > 
> > You might want to do some mass test regeneration and take a look over the 
> > diff to see how useful the changes would be.
> > 
> > If we do change the default, the default change likely need to be part of 
> > `--version 3` to avoid substantial test churn.
> I've regene

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

nikic wrote:
> hnrklssn wrote:
> > nikic wrote:
> > > nikic wrote:
> > > > hnrklssn wrote:
> > > > > hnrklssn wrote:
> > > > > > nikic wrote:
> > > > > > > hnrklssn wrote:
> > > > > > > > delcypher wrote:
> > > > > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what 
> > > > > > > > > `META2` actually refers to. Should we?
> > > > > > > > > 
> > > > > > > > > Not something that has to be fixed in this patch, more just 
> > > > > > > > > an observation.
> > > > > > > > Indeed this is true for metadata in general, presumably because 
> > > > > > > > the RHS often refer to things like other metadata identifiers. 
> > > > > > > > In the case of annotations they seem to always refer to simple 
> > > > > > > > strings however, so it would be feasible to do a straight match 
> > > > > > > > without having to do recursive matching or complex regexes to 
> > > > > > > > determine which part of the metadata to match against.
> > > > > > > > 
> > > > > > > > In many cases with metadata attached to IR nodes, multiple 
> > > > > > > > nodes refer to the same metadata node, so at least you verify 
> > > > > > > > that they still are consistent. But I agree that verifying the 
> > > > > > > > content would be a great future addition.
> > > > > > > You need to pass `--check-globals` to check the actual metadata.
> > > > > > When I add that to this test case it adds
> > > > > > 
> > > > > > ```
> > > > > > //.
> > > > > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > > > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > > > > "stack-protector-buffer-size"="8" 
> > > > > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > > > > //.
> > > > > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > > > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > > > > (g...@github.com:llvm/llvm-project.git 
> > > > > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > > > > // CHECK: !2 = !{!"auto-init"}
> > > > > > //.
> > > > > > ```
> > > > > > 
> > > > > > So it seems to just be doing a simple literal matching on all 
> > > > > > metadata, regardless of whether we captured that metadata in any 
> > > > > > filecheck variable. And it still doesn't use the META2 variable to 
> > > > > > match the definition. Am I missing something? If we use the literal 
> > > > > > metadata names instead of variable matching for the definitions, 
> > > > > > there isn't much point in doing variable matching for the metadata 
> > > > > > uses either, since the test still very much relies on the metadata 
> > > > > > numbering being stable.
> > > > > @nikic Do you have more information to add about how metadata 
> > > > > definition matchers can be generated without hardcoding everything 
> > > > > (which is kind of the opposite of what this patch is trying to do), 
> > > > > or in general if you're happy with the state of the PR?
> > > > This works fine with update_test_checks, so it must be some bug in 
> > > > update_cc_test_checks in particular. From a quick look, I suspect it's 
> > > > because 
> > > > https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
> > > >  hardcodes a True value, while update_test_checks makes this dependent 
> > > > on `--preserve-names`, which is disabled by default there.
> > > Or maybe just test this with update_test_checks? This change is valuable 
> > > for that script as well, and it doesn't have this extra issue.
> > I couldn't find any uses of 
> > `--preserve-names` in the entire repo (not even in tests for 
> > update_test_checks), so I went ahead and changed update_cc_test_checks to 
> > pass True instead.
> > 
> > While at it I added 2 new options to match some, but not necessarily all, 
> > globals. I set the least verbose one (other than "none") as the default, 
> > emitting checks for the definitions of all globals that we emit matches for 
> > in the IR. Do you agree that this is reasonable?
> That sounds like a nice approach. My main question would be whether 
> `--check-globals seen` is the right default: My gut feeling here is that this 
> will introduce additional checks for attributes/metadata in tests that don't 
> actually care about it, but I'm not particularly sure about that.
> 
> You might want to do some mass test regeneration and take a look over the 
> diff to see how useful the changes would be.
> 
> If we do change the default, the default change likely need to be part of 
> `--version 3` to avoid substantial test churn.
I've regenerated checks before and after my patch and compared them. Some 
findings:
 - there's a surprising number of custom metadata types that didn't get 
variable checkers but instead checked hardcoded met

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> nikic wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > hnrklssn wrote:
> > > > > nikic wrote:
> > > > > > hnrklssn wrote:
> > > > > > > delcypher wrote:
> > > > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what 
> > > > > > > > `META2` actually refers to. Should we?
> > > > > > > > 
> > > > > > > > Not something that has to be fixed in this patch, more just an 
> > > > > > > > observation.
> > > > > > > Indeed this is true for metadata in general, presumably because 
> > > > > > > the RHS often refer to things like other metadata identifiers. In 
> > > > > > > the case of annotations they seem to always refer to simple 
> > > > > > > strings however, so it would be feasible to do a straight match 
> > > > > > > without having to do recursive matching or complex regexes to 
> > > > > > > determine which part of the metadata to match against.
> > > > > > > 
> > > > > > > In many cases with metadata attached to IR nodes, multiple nodes 
> > > > > > > refer to the same metadata node, so at least you verify that they 
> > > > > > > still are consistent. But I agree that verifying the content 
> > > > > > > would be a great future addition.
> > > > > > You need to pass `--check-globals` to check the actual metadata.
> > > > > When I add that to this test case it adds
> > > > > 
> > > > > ```
> > > > > //.
> > > > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > > > "stack-protector-buffer-size"="8" 
> > > > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > > > //.
> > > > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > > > (g...@github.com:llvm/llvm-project.git 
> > > > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > > > // CHECK: !2 = !{!"auto-init"}
> > > > > //.
> > > > > ```
> > > > > 
> > > > > So it seems to just be doing a simple literal matching on all 
> > > > > metadata, regardless of whether we captured that metadata in any 
> > > > > filecheck variable. And it still doesn't use the META2 variable to 
> > > > > match the definition. Am I missing something? If we use the literal 
> > > > > metadata names instead of variable matching for the definitions, 
> > > > > there isn't much point in doing variable matching for the metadata 
> > > > > uses either, since the test still very much relies on the metadata 
> > > > > numbering being stable.
> > > > @nikic Do you have more information to add about how metadata 
> > > > definition matchers can be generated without hardcoding everything 
> > > > (which is kind of the opposite of what this patch is trying to do), or 
> > > > in general if you're happy with the state of the PR?
> > > This works fine with update_test_checks, so it must be some bug in 
> > > update_cc_test_checks in particular. From a quick look, I suspect it's 
> > > because 
> > > https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
> > >  hardcodes a True value, while update_test_checks makes this dependent on 
> > > `--preserve-names`, which is disabled by default there.
> > Or maybe just test this with update_test_checks? This change is valuable 
> > for that script as well, and it doesn't have this extra issue.
> I couldn't find any uses of 
> `--preserve-names` in the entire repo (not even in tests for 
> update_test_checks), so I went ahead and changed update_cc_test_checks to 
> pass True instead.
> 
> While at it I added 2 new options to match some, but not necessarily all, 
> globals. I set the least verbose one (other than "none") as the default, 
> emitting checks for the definitions of all globals that we emit matches for 
> in the IR. Do you agree that this is reasonable?
That sounds like a nice approach. My main question would be whether 
`--check-globals seen` is the right default: My gut feeling here is that this 
will introduce additional checks for attributes/metadata in tests that don't 
actually care about it, but I'm not particularly sure about that.

You might want to do some mass test regeneration and take a look over the diff 
to see how useful the changes would be.

If we do change the default, the default change likely need to be part of 
`--version 3` to avoid substantial test churn.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

nikic wrote:
> nikic wrote:
> > hnrklssn wrote:
> > > hnrklssn wrote:
> > > > nikic wrote:
> > > > > hnrklssn wrote:
> > > > > > delcypher wrote:
> > > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` 
> > > > > > > actually refers to. Should we?
> > > > > > > 
> > > > > > > Not something that has to be fixed in this patch, more just an 
> > > > > > > observation.
> > > > > > Indeed this is true for metadata in general, presumably because the 
> > > > > > RHS often refer to things like other metadata identifiers. In the 
> > > > > > case of annotations they seem to always refer to simple strings 
> > > > > > however, so it would be feasible to do a straight match without 
> > > > > > having to do recursive matching or complex regexes to determine 
> > > > > > which part of the metadata to match against.
> > > > > > 
> > > > > > In many cases with metadata attached to IR nodes, multiple nodes 
> > > > > > refer to the same metadata node, so at least you verify that they 
> > > > > > still are consistent. But I agree that verifying the content would 
> > > > > > be a great future addition.
> > > > > You need to pass `--check-globals` to check the actual metadata.
> > > > When I add that to this test case it adds
> > > > 
> > > > ```
> > > > //.
> > > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > > "stack-protector-buffer-size"="8" 
> > > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > > //.
> > > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > > (g...@github.com:llvm/llvm-project.git 
> > > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > > // CHECK: !2 = !{!"auto-init"}
> > > > //.
> > > > ```
> > > > 
> > > > So it seems to just be doing a simple literal matching on all metadata, 
> > > > regardless of whether we captured that metadata in any filecheck 
> > > > variable. And it still doesn't use the META2 variable to match the 
> > > > definition. Am I missing something? If we use the literal metadata 
> > > > names instead of variable matching for the definitions, there isn't 
> > > > much point in doing variable matching for the metadata uses either, 
> > > > since the test still very much relies on the metadata numbering being 
> > > > stable.
> > > @nikic Do you have more information to add about how metadata definition 
> > > matchers can be generated without hardcoding everything (which is kind of 
> > > the opposite of what this patch is trying to do), or in general if you're 
> > > happy with the state of the PR?
> > This works fine with update_test_checks, so it must be some bug in 
> > update_cc_test_checks in particular. From a quick look, I suspect it's 
> > because 
> > https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
> >  hardcodes a True value, while update_test_checks makes this dependent on 
> > `--preserve-names`, which is disabled by default there.
> Or maybe just test this with update_test_checks? This change is valuable for 
> that script as well, and it doesn't have this extra issue.
I couldn't find any uses of 
`--preserve-names` in the entire repo (not even in tests for 
update_test_checks), so I went ahead and changed update_cc_test_checks to pass 
True instead.

While at it I added 2 new options to match some, but not necessarily all, 
globals. I set the least verbose one (other than "none") as the default, 
emitting checks for the definitions of all globals that we emit matches for in 
the IR. Do you agree that this is reasonable?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 525174.
hnrklssn added a comment.
Herald added a subscriber: ormris.

Overhauled how update_cc_tests.py checks globals

It now:

- no longer matches the literal names, like update_test_checks.py does unless 
--preserve-names (which doesn't seem to see much use, at least not upstream, so 
did not add option to update_cc_tests.py) is set
- (along with update_test_checks.py) now adds checks for any definitions of 
globals referenced in the IR (by default, can still check all globals using 
"--check-globals all")
- (along with update_test_checks.py) can also be configured to emit checks for 
any definitions of globals referenced by other globals referenced in the IR, 
using "--check-globals transitive"
- (along with update_test_checks.py) filters out unstable metadata output, so 
that literal directory names and git hashes are not matched against


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  clang/test/utils/update_cc_test_checks/Inputs/basic-cplusplus.cpp.expected
  
clang/test/utils/update_cc_test_checks/Inputs/explicit-template-instantiation.cpp.expected
  clang/test/utils/update_cc_test_checks/Inputs/generated-funcs-regex.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/generated-funcs.c.no-generated.expected
  
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/Inputs/mangled_names.c.funcsig.expected
  
clang/test/utils/update_cc_test_checks/Inputs/mangled_names.c.funcsig.v2.expected
  clang/test/utils/update_cc_test_checks/Inputs/mangled_names.c.v2.expected
  
clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c.expected
  
clang/test/utils/update_cc_test_checks/Inputs/resolve-tmp-conflict.cpp.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/generated-funcs.test
  clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
  clang/test/utils/update_cc_test_checks/global-value-regex.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.plain.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.generated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/generated_funcs_prefix_reuse.ll.nogenerated.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/pre-process.ll.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/scrub_attrs.ll.plain.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.globals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.noglobals.expected
  
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/various_ir_values.ll.funcsig.transitiveglobals.expected
  llvm/test/tools/UpdateTestChecks/update_test_checks/generated_funcs.test
  
llvm/test/tools/UpdateTestChecks/update_test_checks/generated_funcs_prefix_reuse.test
  llvm/test/tools/UpdateTestChecks/update_test_checks/various_ir_values.test
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py
  llvm/utils/update_test_checks.py

Index: llvm/utils/update_test_checks.py
===
--- llvm/utils/update_test_checks.py
+++ llvm/utils/update_test_checks.py
@@ -59,7 +59,7 @@
   help='Remove attribute annotations (#0) from the end of check line')
   parser.add_argument('--check-attributes', action='store_true',
 

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

nikic wrote:
> hnrklssn wrote:
> > hnrklssn wrote:
> > > nikic wrote:
> > > > hnrklssn wrote:
> > > > > delcypher wrote:
> > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` 
> > > > > > actually refers to. Should we?
> > > > > > 
> > > > > > Not something that has to be fixed in this patch, more just an 
> > > > > > observation.
> > > > > Indeed this is true for metadata in general, presumably because the 
> > > > > RHS often refer to things like other metadata identifiers. In the 
> > > > > case of annotations they seem to always refer to simple strings 
> > > > > however, so it would be feasible to do a straight match without 
> > > > > having to do recursive matching or complex regexes to determine which 
> > > > > part of the metadata to match against.
> > > > > 
> > > > > In many cases with metadata attached to IR nodes, multiple nodes 
> > > > > refer to the same metadata node, so at least you verify that they 
> > > > > still are consistent. But I agree that verifying the content would be 
> > > > > a great future addition.
> > > > You need to pass `--check-globals` to check the actual metadata.
> > > When I add that to this test case it adds
> > > 
> > > ```
> > > //.
> > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > "stack-protector-buffer-size"="8" 
> > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > //.
> > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > (g...@github.com:llvm/llvm-project.git 
> > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > // CHECK: !2 = !{!"auto-init"}
> > > //.
> > > ```
> > > 
> > > So it seems to just be doing a simple literal matching on all metadata, 
> > > regardless of whether we captured that metadata in any filecheck 
> > > variable. And it still doesn't use the META2 variable to match the 
> > > definition. Am I missing something? If we use the literal metadata names 
> > > instead of variable matching for the definitions, there isn't much point 
> > > in doing variable matching for the metadata uses either, since the test 
> > > still very much relies on the metadata numbering being stable.
> > @nikic Do you have more information to add about how metadata definition 
> > matchers can be generated without hardcoding everything (which is kind of 
> > the opposite of what this patch is trying to do), or in general if you're 
> > happy with the state of the PR?
> This works fine with update_test_checks, so it must be some bug in 
> update_cc_test_checks in particular. From a quick look, I suspect it's 
> because 
> https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
>  hardcodes a True value, while update_test_checks makes this dependent on 
> `--preserve-names`, which is disabled by default there.
Or maybe just test this with update_test_checks? This change is valuable for 
that script as well, and it doesn't have this extra issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> hnrklssn wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > delcypher wrote:
> > > > > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` 
> > > > > actually refers to. Should we?
> > > > > 
> > > > > Not something that has to be fixed in this patch, more just an 
> > > > > observation.
> > > > Indeed this is true for metadata in general, presumably because the RHS 
> > > > often refer to things like other metadata identifiers. In the case of 
> > > > annotations they seem to always refer to simple strings however, so it 
> > > > would be feasible to do a straight match without having to do recursive 
> > > > matching or complex regexes to determine which part of the metadata to 
> > > > match against.
> > > > 
> > > > In many cases with metadata attached to IR nodes, multiple nodes refer 
> > > > to the same metadata node, so at least you verify that they still are 
> > > > consistent. But I agree that verifying the content would be a great 
> > > > future addition.
> > > You need to pass `--check-globals` to check the actual metadata.
> > When I add that to this test case it adds
> > 
> > ```
> > //.
> > // CHECK: attributes #0 = { noinline nounwind optnone 
> > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > "stack-protector-buffer-size"="8" 
> > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > //.
> > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > // CHECK: !1 = !{!"clang version 17.0.0 
> > (g...@github.com:llvm/llvm-project.git 
> > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > // CHECK: !2 = !{!"auto-init"}
> > //.
> > ```
> > 
> > So it seems to just be doing a simple literal matching on all metadata, 
> > regardless of whether we captured that metadata in any filecheck variable. 
> > And it still doesn't use the META2 variable to match the definition. Am I 
> > missing something? If we use the literal metadata names instead of variable 
> > matching for the definitions, there isn't much point in doing variable 
> > matching for the metadata uses either, since the test still very much 
> > relies on the metadata numbering being stable.
> @nikic Do you have more information to add about how metadata definition 
> matchers can be generated without hardcoding everything (which is kind of the 
> opposite of what this patch is trying to do), or in general if you're happy 
> with the state of the PR?
This works fine with update_test_checks, so it must be some bug in 
update_cc_test_checks in particular. From a quick look, I suspect it's because 
https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
 hardcodes a True value, while update_test_checks makes this dependent on 
`--preserve-names`, which is disabled by default there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> nikic wrote:
> > hnrklssn wrote:
> > > delcypher wrote:
> > > > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` 
> > > > actually refers to. Should we?
> > > > 
> > > > Not something that has to be fixed in this patch, more just an 
> > > > observation.
> > > Indeed this is true for metadata in general, presumably because the RHS 
> > > often refer to things like other metadata identifiers. In the case of 
> > > annotations they seem to always refer to simple strings however, so it 
> > > would be feasible to do a straight match without having to do recursive 
> > > matching or complex regexes to determine which part of the metadata to 
> > > match against.
> > > 
> > > In many cases with metadata attached to IR nodes, multiple nodes refer to 
> > > the same metadata node, so at least you verify that they still are 
> > > consistent. But I agree that verifying the content would be a great 
> > > future addition.
> > You need to pass `--check-globals` to check the actual metadata.
> When I add that to this test case it adds
> 
> ```
> //.
> // CHECK: attributes #0 = { noinline nounwind optnone 
> "min-legal-vector-width"="0" "no-trapping-math"="true" 
> "stack-protector-buffer-size"="8" 
> "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> //.
> // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> // CHECK: !1 = !{!"clang version 17.0.0 
> (g...@github.com:llvm/llvm-project.git 
> 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> // CHECK: !2 = !{!"auto-init"}
> //.
> ```
> 
> So it seems to just be doing a simple literal matching on all metadata, 
> regardless of whether we captured that metadata in any filecheck variable. 
> And it still doesn't use the META2 variable to match the definition. Am I 
> missing something? If we use the literal metadata names instead of variable 
> matching for the definitions, there isn't much point in doing variable 
> matching for the metadata uses either, since the test still very much relies 
> on the metadata numbering being stable.
@nikic Do you have more information to add about how metadata definition 
matchers can be generated without hardcoding everything (which is kind of the 
opposite of what this patch is trying to do), or in general if you're happy 
with the state of the PR?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

nikic wrote:
> hnrklssn wrote:
> > delcypher wrote:
> > > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` 
> > > actually refers to. Should we?
> > > 
> > > Not something that has to be fixed in this patch, more just an 
> > > observation.
> > Indeed this is true for metadata in general, presumably because the RHS 
> > often refer to things like other metadata identifiers. In the case of 
> > annotations they seem to always refer to simple strings however, so it 
> > would be feasible to do a straight match without having to do recursive 
> > matching or complex regexes to determine which part of the metadata to 
> > match against.
> > 
> > In many cases with metadata attached to IR nodes, multiple nodes refer to 
> > the same metadata node, so at least you verify that they still are 
> > consistent. But I agree that verifying the content would be a great future 
> > addition.
> You need to pass `--check-globals` to check the actual metadata.
When I add that to this test case it adds

```
//.
// CHECK: attributes #0 = { noinline nounwind optnone 
"min-legal-vector-width"="0" "no-trapping-math"="true" 
"stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" 
}
//.
// CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
// CHECK: !1 = !{!"clang version 17.0.0 (g...@github.com:llvm/llvm-project.git 
684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
// CHECK: !2 = !{!"auto-init"}
//.
```

So it seems to just be doing a simple literal matching on all metadata, 
regardless of whether we captured that metadata in any filecheck variable. And 
it still doesn't use the META2 variable to match the definition. Am I missing 
something? If we use the literal metadata names instead of variable matching 
for the definitions, there isn't much point in doing variable matching for the 
metadata uses either, since the test still very much relies on the metadata 
numbering being stable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> delcypher wrote:
> > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` actually 
> > refers to. Should we?
> > 
> > Not something that has to be fixed in this patch, more just an observation.
> Indeed this is true for metadata in general, presumably because the RHS often 
> refer to things like other metadata identifiers. In the case of annotations 
> they seem to always refer to simple strings however, so it would be feasible 
> to do a straight match without having to do recursive matching or complex 
> regexes to determine which part of the metadata to match against.
> 
> In many cases with metadata attached to IR nodes, multiple nodes refer to the 
> same metadata node, so at least you verify that they still are consistent. 
> But I agree that verifying the content would be a great future addition.
You need to pass `--check-globals` to check the actual metadata.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

delcypher wrote:
> @hnrklssn I just noticed we don't have a `CHECK` for what `META2` actually 
> refers to. Should we?
> 
> Not something that has to be fixed in this patch, more just an observation.
Indeed this is true for metadata in general, presumably because the RHS often 
refer to things like other metadata identifiers. In the case of annotations 
they seem to always refer to simple strings however, so it would be feasible to 
do a straight match without having to do recursive matching or complex regexes 
to determine which part of the metadata to match against.

In many cases with metadata attached to IR nodes, multiple nodes refer to the 
same metadata node, so at least you verify that they still are consistent. But 
I agree that verifying the content would be a great future addition.



Comment at: llvm/utils/UpdateTestChecks/common.py:703
   def get_ir_prefix_from_ir_value_match(self, match):
-return self.ir_prefix, self.check_prefix
+return re.search(self.ir_prefix, match[0])[0], self.check_prefix
 

delcypher wrote:
> Is this call to `re.search(...)` guaranteed to always succeed?
Yes. `match` is a match object from a combined regex concatenated from both the 
IR prefix (`![a-z.]+ `) and the IR regexp (`![0-9]+`), so it will always 
contain a substring matching the IR prefix.



Comment at: llvm/utils/UpdateTestChecks/common.py:779
 NamelessValue(r'ACC_GRP', '!' , r'!llvm.access.group ' , r'![0-9]+'
 , None ) ,
+NamelessValue(r'META'   , '!' , r'![a-z.]+ '   , r'![0-9]+'
 , None ) ,
 ]

delcypher wrote:
> There may be some value in having `!annotations` matched explicitly as well 
> as there being a fallback. In the current patch it looks like:
> 
> * `metadata` is assigned variables with the `META` prefix on `CHECK` lines
> * `!annotation` is assigned variables with the `META` prefix on `CHECK` lines
> * Anything else that matches `r'![a-z.]+ ' `  is assigned variables with the 
> `META` prefix on `CHECK` lines
> 
> When looking a large test having everything lumped into `META` variables 
> could make the generated tests a little harder to read.
In a way, yes. At the same time, since we're not matching against the 
definition of the metadata node, all uses (while named META#) will still be 
prefixed by !annotation. On the other hand again, we have explicitly enumerated 
so many other types of metadata, so it is a bit inconsistent.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Dan Liew via Phabricator via cfe-commits]

delcypher added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

@hnrklssn I just noticed we don't have a `CHECK` for what `META2` actually 
refers to. Should we?

Not something that has to be fixed in this patch, more just an observation.



Comment at: llvm/utils/UpdateTestChecks/common.py:703
   def get_ir_prefix_from_ir_value_match(self, match):
-return self.ir_prefix, self.check_prefix
+return re.search(self.ir_prefix, match[0])[0], self.check_prefix
 

Is this call to `re.search(...)` guaranteed to always succeed?



Comment at: llvm/utils/UpdateTestChecks/common.py:779
 NamelessValue(r'ACC_GRP', '!' , r'!llvm.access.group ' , r'![0-9]+'
 , None ) ,
+NamelessValue(r'META'   , '!' , r'![a-z.]+ '   , r'![0-9]+'
 , None ) ,
 ]

There may be some value in having `!annotations` matched explicitly as well as 
there being a fallback. In the current patch it looks like:

* `metadata` is assigned variables with the `META` prefix on `CHECK` lines
* `!annotation` is assigned variables with the `META` prefix on `CHECK` lines
* Anything else that matches `r'![a-z.]+ ' `  is assigned variables with the 
`META` prefix on `CHECK` lines

When looking a large test having everything lumped into `META` variables could 
make the generated tests a little harder to read.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added a comment.

In D148216#4266201 , @nikic wrote:

> We should probably just use the `META` fallback for all metadata not already 
> explicitly handled. I think there's pretty little value in adding a special 
> name for each metadata kind.

I added a fallback, and some logic to extract the metadata kind name from the 
IR, because I do find it useful to emit checks named after the type of IR they 
match against.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn added a comment.

In D148216#4266174 , @arichardson 
wrote:

> Change looks good to me, but if clang can't emit these yet we should probably 
> wait until committing this? Do you have a link to a review that uses this 
> metadata?

It can emit them, we just use them a bit more frequently downstream. I've found 
an example and added a lit test as an example of how it works.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Henrik G Olsson via Phabricator via cfe-commits]

hnrklssn updated this revision to Diff 513589.
hnrklssn added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add test case emitting !annotation from clang.
Make annotation matching a generic metadata fallback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148216/new/

https://reviews.llvm.org/D148216

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c
  clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
  clang/test/utils/update_cc_test_checks/annotations.test
  llvm/utils/UpdateTestChecks/common.py


Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -697,9 +697,10 @@
 return self.global_ir_rhs_regexp is not None
 
   # Return the IR prefix and check prefix we use for this kind or IR value,
-  # e.g., (%, TMP) for locals.
+  # e.g., (%, TMP) for locals. If the IR prefix is a regex, return the prefix
+  # used in the IR output
   def get_ir_prefix_from_ir_value_match(self, match):
-return self.ir_prefix, self.check_prefix
+return re.search(self.ir_prefix, match[0])[0], self.check_prefix
 
   # Return the IR regexp we use for this kind or IR value, e.g., [\w.-]+? for 
locals
   def get_ir_regex_from_ir_value_re_match(self, match):
@@ -775,6 +776,7 @@
 NamelessValue(r'META'   , '!' , r'metadata '   , r'![0-9]+'
 , None ) ,
 NamelessValue(r'META'   , '!' , r'', r'![0-9]+'
 , r'(?:distinct |)!.*' ) ,
 NamelessValue(r'ACC_GRP', '!' , r'!llvm.access.group ' , r'![0-9]+'
 , None ) ,
+NamelessValue(r'META'   , '!' , r'![a-z.]+ '   , r'![0-9]+'
 , None ) ,
 ]
 
 asm_nameless_values = [
Index: clang/test/utils/update_cc_test_checks/annotations.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/annotations.test
@@ -0,0 +1,4 @@
+## Test that !annotation metadata is matched correctly
+
+# RUN: cp %S/Inputs/annotations.c %t.c && %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/annotations.c.expected %t.c
Index: clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected
@@ -0,0 +1,17 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks 
-ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s
+
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[X:%.*]] = alloca i32, align 4
+// CHECK-NEXT:store i32 0, ptr [[X]], align 4, !annotation 
[[META2:![0-9]+]]
+// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:[[ADD:%.*]] = add nsw i32 [[TMP0]], 1
+// CHECK-NEXT:store i32 [[ADD]], ptr [[X]], align 4
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//
+int foo() {
+int x = x + 1;
+return x;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/annotations.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/annotations.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks 
-ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s
+
+int foo() {
+int x = x + 1;
+return x;
+}


Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -697,9 +697,10 @@
 return self.global_ir_rhs_regexp is not None
 
   # Return the IR prefix and check prefix we use for this kind or IR value,
-  # e.g., (%, TMP) for locals.
+  # e.g., (%, TMP) for locals. If the IR prefix is a regex, return the prefix
+  # used in the IR output
   def get_ir_prefix_from_ir_value_match(self, match):
-return self.ir_prefix, self.check_prefix
+return re.search(self.ir_prefix, match[0])[0], self.check_prefix
 
   # Return the IR regexp we use for this kind or IR value, e.g., [\w.-]+? for locals
   def get_ir_regex_from_ir_value_re_match(self, match):
@@ -775,6 +776,7 @@
 NamelessValue(r'META'   , '!' , r'metadata '   , r'![0-9]+' , None ) ,
 NamelessValue(r'META'   , '!' , r'', r'![0-9]+' , r'(?:distinct |)!.*' ) ,
 NamelessValue(r'ACC_GRP', '!' , r'!llvm.access.group ' , r'![0-9]+' , None ) ,
+NamelessValue(r'META'   , '!' , r'![a-z.]+ '   , r'![0-9]+' , None