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
