[PATCH] D118416: [Metadata] Use temporary MD nodes when appending module flags during module linking

Fri Mar 25 11:34:55 PDT 2022

wolfgangp added a comment.
Herald added a project: All.

I'm finally getting around to take a stab at this. One thing in particular is giving me trouble:

In D118416#3285239 <https://reviews.llvm.org/D118416#3285239>, @dexonsmith wrote:

> 

> - Ensure that an empty temporary or distinct node can be resized later by co-allocating enough space; as a side effect, even an empty node would start with some small storage, but that's probably okay. (An empty uniqued node wouldn't need anything co-allocated.)

I'm largely following your implementation strategy. On first allocation I'm creating all nodes they way they are currently, i.e. with co-allocated operands. For temporary and distinct nodes I'm allocating extra space for a potentially needed hung-off-storage descriptor, if the number of operands is small (i.e. the space allocated for them is not enough for what's needed for the descriptor). For uniqued nodes I don't allocate anything extra.

The resize operation is fairly straightforward to implement, but the trouble I'm having is at deallocation. Temporary MDnodes can be made unique, and later at deallocation time we can't distinguish between uniqued nodes that have originally been allocated as temporary nodes and the ones that were allocated as unique from the start. A possible solution would be to steal another bit from NumOperands (reducing it to 30 bits) and capture the original allocation state there, but I'm still looking for alternatives.

Cloning these small temporary nodes (without the extra size) at uniquing time would be another option, though it seems like the infrastructure is not available for this operation. I assume we don't want to saddle all MDnodes (even the uniqued ones) with the extra space.

At the moment I'm trying the additional bit approach, which seems to be working, but I wouldn't mind hearing your thoughts on this.

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

https://reviews.llvm.org/D118416