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

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

dexonsmith added a comment.

In D118416#3408550 <https://reviews.llvm.org/D118416#3408550>, @wolfgangp wrote:

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

Stealing a bit down to NumOperands=30 seems pretty reasonable. I think an MDNode with more than a billion operands probably doesn't need to be supported.

I think it's okay to saddle anything that came through the "temporary" or "distinct" path with one extra word; not that bitcode layout of metadata is designed to avoid metadata temporaries in most cases for uniqued nodes.

But I'm not sure you need it. I think you can do something like this pseudo-code for the co-allocation:

  struct HungOffType {
    uint32_t Capacity;
    uint32_t NumOperands;
    MDOperand Ops[Capacity];
  };
  union CoallocationType {
    MDOperand Small[SmallCapacity];
    std::unique_ptr<HungOffType> Large;
  };

You can cut up `NumOperands` without loss of generality once you have that setup:

  uint32_t IsSmall : 1;          // Whether it's small.
  uint32_t SmallCapacity : 8;    // Co-allocate up to 256 operands.
  uint32_t SmallNumOperands : 8; // How much small storage is used?
  
  size_t getNumOperands() const {
    return IsSmall ? SmallNumOperands : getLargeOps().NumOperands;
  }

This limits the co-allocation to 256 operands, but if the initial number of operands is too big for that it can hang them off (maybe we want to limit small size further; a histogram might tell us). It also buries `getNumOperands()` behind a an extra pointer, but anyone checking the size is probably iterating through anyway so it's a pointer they were already going to traverse.


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

https://reviews.llvm.org/D118416