[PATCH] D150370: Introduce StructuredData

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Thanks for taking a look!

In D150370#4349276 <https://reviews.llvm.org/D150370#4349276>, @Flakebi wrote:

> Why does the symbol map need to be global, shouldn’t it by per LLVMContext (which would also get rid of the locking)?

It's global so that we can cheaply compare an `sdata::Symbol` against an `sdata::RegisterSymbol`. This is used e.g. here: https://github.com/nhaehnle/llvm-project/blob/6bb5923a059c1120e006cb0d0cd63c5ef0806c0e/llvm/lib/IR/Type.cpp#L844

> Needs tests.

What kind of tests do you have in mind? This change doesn't really expose anything, and there are tests in the followup change in the stack.



================
Comment at: llvm/docs/LangRef.rst:3519-3523
+    sdata       ::= '{' (sdata_field ',')* sdata_field? '}'
+    sdata_field ::= label sdata_value
+    sdata_value ::= 'type' type
+                ::= integer
+                ::= 'true' | 'false'
----------------
Flakebi wrote:
> Can constants follow the way metadata (and most other things in LLVM) is encoded and prefix the type?
> I.e. `i32 <integer>` and `i1 true/false`
Good question. I've been going back and forth on this, and the current version is as-is mostly because the various !DIxyz metadata doesn't have those prefixes. But that may not be the best example to follow.

I'd be happy to change it to be more in line with metadata. Any other opinions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150370