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