IR: Add distinct MDNodes as a first-class concept

[Rafael Espíndola]

On 8 January 2015 at 11:31, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015 Jan 8, at 06:28, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>>
>>> Introduce a `distinct` designator which explicitly requests this
>>> behaviour, without the need for a self-reference.  These nodes are
>>> still stored in the `LLVMContext`, but are not uniqued based on
>>> their operands (well, not uniqued at all).
>>>
>>>  - This concept *already exists*.  Before the metadata/value split,
>>>    it was used whenever operands went to null (to prevent teardown
>>>    madness, but it happened more frequently).  It's still in use
>>>    for self-referencing `MDNode`s, as well as when
>>>    `MDNode::replaceOperandWith()` causes a uniquing collision (see
>>>    module flags behaviour before r225397 for an example).
>>
>> Side question: this is because of missing rauw on resolved nodes, right?
>
> It existed before whenever an operand went to null.  It exists now
> because of `replaceOperandWith()` *and* so that metadata schemas that
> use self-references don't need to call `resolveCycles()`.
>
> There are two reasons I think this should be first class.
>
>  1. Self-references are an awkward, gross way to prevent uniquing.  This
>     is better.
>
>  2. Uniquing collisions shouldn't cause unreproducible IR.
>
>> Could we just disallow replaceOperandWith on resolved nodes and add a
>> "MDNode *getNodeWithReplacedOperand(unsigned I, Metadata *New)" that
>> returns a node just like 'this' but with operand I replaced with New?
>
> This is a good idea -- I'd like to do this -- but it doesn't eliminate
> uniquing collisions.
>
> Uniquing collisions also happen when an operand that does support RAUW
> gets replaced.  For example:
>
>     !0 = !{i32* @g1}
>     !1 = !{i32* @g2}
>
> After `@g1->replaceAllUsesWith(@g2)`:
>
>     !0 = distinct !{i32* @g2} ; Collision with !1
>     !1 = !{i32* @g2}
>
> (This is a bit of a "grass is greener" situation where we really want
> RAUW, if only we could afford it.  However, I expect uniquing
> collisions are more rare than operands going to null, which is what
> dropped uniqueness before the metadata/value split.  For example:
>
>     !0 = !{..., i32* @g, ...}
>
> After `delete @g`:
>
>     !0 = distinct !{..., null, ...} ; Operand went to null
>
> I think in practice we're in better shape than we used to be.)

I agree. I also agree that making the 'distinct' bit explicit is a
good thing. It was really just a side question about the API :-)

Cheers,
Rafael