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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D78793#2424081 <https://reviews.llvm.org/D78793#2424081>, @nicholas wrote:

> 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.

Fair enough, makes sense to me.

>> 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.

Yeah, that's exactly what's fixed by the opaque pointer work: it turns the type graph into a DAG. In your example, the type wasn't holding itself as a member (that's not possible), it was holding a pointer to itself as a member. Once pointers are opaque, then it's just holding a pointer, no cycles anymore.

>> 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?

Sorry, the objects held by the LLVMContext would be immutable; adding things to it could certainly race; but yeah, I think you can do that without a lock.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78793