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
