[PATCH] D78793: Names for structs are held on the Context, not the Module. Move getTypeByName from Module to Type taking a Context parameter.

[Nick Lewycky via Phabricator via llvm-commits]

nicholas added a comment.

In D78793#2424013 <https://reviews.llvm.org/D78793#2424013>, @dexonsmith wrote:

> @nicholas, I missed this when it was reviewed, but seeing it now that you've landed it.
>
> Can you help me understand if there's a deeper motivation than the one in the patch description? I ask because I would have envisioned eventually moving these "named" types to the module (i.e., keep the old API before your change, but change the implementation to make sense of it).

My motivation really is only what's stated in the description, I was asked about this by an author of an LLVM wrapper for Rust, all the other APIs didn't need a Module and this one did. If I remember right, LLVM pre-3.0 used to work as you describe, at which point the current system where distinct names of structs imply distinct struct types was introduced leading to a major simplification / performance improvement in forward declarations and module linking. See the first bullet under "Internal API Changes" in https://releases.llvm.org/3.0/docs/ReleaseNotes.html . My justification for this patch is that it's just cleaning up this one spot that was missed way back in the 2.9 -> 3.0 cycle.

> In particular, it would be nice if LLVMContext were restricted to types / values / metadata that are purely functional, and anything non-functional or open to interpretation is put on the Module.
>
> - For values and metadata, this requires some significant work (I have some concrete ideas of how, but it seems quite tangential to this patch).
> - For types, this could be done as a very simple follow-up to the opaque pointer work (@dblaikie @arsenm, not sure if you have thoughts). The `Type` class would be purely functional, and the struct names would move to a symbol table in each `Module`. The name is not inherent to the type, it's just a handle that some module wants to use to refer to it (the type itself can be shared between modules even if they disagree about what to call it).

I don't have much to add here, I'd simply recommend that you have a plan for dealing with the problems of type merging and refinement, structs whose members are pointers to that same struct (detect and reject creating a struct that holds itself as a member, `%0 = type { \0 }`), etc., before making the change.

I'm not familiar with the opaque pointer work and how it affects the types and context.

> One benefit of this hypothetical change is that we could change everything in `LLVMContext` to be immutable, potentially using lock-free data structures, etc.; I think that would open up some interesting possibilities.

Offhand it's not clear to me that the `LLVMContext` becomes immutable? It still has to construct those pure-functional objects exactly once to ensure they're pointer-comparable, and that's a mutation? Perhaps you can do that without a lock, and that's close enough?



================
Comment at: llvm/include/llvm-c/Core.h:631
+ */
+LLVMTypeRef LLVMGetTypeByName2(LLVMContextRef C, const char *Name);
+
----------------
nicholas wrote:
> clang-tidy readability-identifier-naming doesn't like 'LLVMGetTypeByName2'. Should I ignore this because it's standard style for the LLVM C API? Or what should I change it to?
> clang-tidy readability-identifier-naming doesn't like 'LLVMGetTypeByName2'. Should I ignore this because it's standard style for the LLVM C API? Or what should I change it to?

It appears that this blocks buildbot due to the clang-tidy error. I'll try suppressing with `NOLINT` now, but it'd be nice if the automated checks were aware of the C API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78793