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