<div dir="ltr">+Duncan in case he has any concerns/ideas/verification for any of this.<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 1, 2016 at 5:11 PM Artem Belevich <<a href="mailto:tra@google.com">tra@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">tra added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D22837#502775" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22837#502775</a>, @dblaikie wrote:<br>
<br>
> "As far as I can tell, explicitly updating named metadata is not necessary -- updating metadata nodes for variables we've changed appears to maintain metadata consistency. I didn't manage to find a good way to verify it, though. If anyone has suggestions how to reference variable from named metadata, I can add it to the new test."<br>
><br>
> Not sure I understand what you were and were not able to verify/test. Could you explain that more/differently?<br>
<br>
<br>
I've verified that DI metadata is consistent after the pass. I.e. llvm.dbg.cu->DICompileUnit:globals->!3->DIGlobalVariable->correct reference to @static_var. Implicitly IR verification pass also verified that there's no leftover DICompileUnits hanging around which was the case before the patch.<br></blockquote><div><br></div><div>But the reason that happened was because the Subprogram referenced the original CU, right? So I'd expect to just test that chain: Function->subprogram->CU-> cu named metadata list.<br><br>Could you just test that?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
What I'm not sure about is whether named metadata will always be handled correctly.<br>
As far as I can tell, named metadata is pretty much read-only. You can only replace it with a new temporary instance.<br>
Removed code was attempting to manually update named metadata if it happened to reference (directly or not) any IR that the pass changed.<br>
<br>
Currently all named metadata is few references away from IR that gets changed. Regular metadata nodes can be modified (all?, some?), so changing a leaf node does not necessarily require replacement of the metadata node that references it. Updating single metadata node appears to be sufficient and does not require to propagate changes further.<br>
<br>
However, if there's a way for a global node to refer to IR, that may need explicit replacement of such node.<br>
<br>
> The test case looks fairly reasonable to me - but I also wouldn't mind an explanation of all the features (why the artificial/named metadata node? I would expect to just match the CU/Globals/global metadata nodes, and check that the reference (as you have) uses whatever is necessary (the addrspace casts, etc) to still reference that global)<br>
<br>
<br>
Artificial named node is a placeholder for a named metadata node that would have something that is modified by the pass and thus may need to be replaced. So far I've failed to create such metadata.<br></blockquote><div><br></div><div>Hopefully Duncan can back (or refute) me up here - but I'd say just remove that as unneeded/not interesting to the reproduction. It seems no different than the actual debug info metadata - a named node that indirectly references the global & gets updated as expected.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">LLVM language reference says that named metadata can only have node operands, which guarantees at least one level of indirection, so named node can't refer to IR directly (i.e. it can't have DIGlobalVariable operands.). This simplifies things somehow.<br>
<br>
What I need to know is -- when a regular metadata node is modified, am I guaranteed that:<br>
a) either modification happend in-place, without replacing the node,<br>
b) or, if the node is replaced, all metadata nodes referencing it (including named metadata nodes) are updated appropriately.<br>
<br>
If that is true, then we're good. It appears to be the case, but I can't tell if I'm just missing a test case that would prove me wrong.<br>
<br>
I guess I can try to see what happens when I apply ReplaceAllUsesWith() to a node used as an operand of named metadata. If that works correctly (i.e it would update named metadata operand to point to the new node), then I'm in the clear. If it does not, then I'll need to figure out how such case should be handled.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D22837" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22837</a><br>
<br>
<br>
<br>
</blockquote></div></div>