[PATCH] D22837: [NVPTX] remove unnecessary named metadata update that happens to break debug info.

Tue Aug 2 11:54:30 PDT 2016

> On 2016-Aug-02, at 10:15, David Blaikie <dblaikie at gmail.com> wrote:
> 
> +Duncan in case he has any concerns/ideas/verification for any of this.
> 
> On Mon, Aug 1, 2016 at 5:11 PM Artem Belevich <tra at google.com <mailto:tra at google.com>> wrote:
> tra added a comment.
> 
> In https://reviews.llvm.org/D22837#502775 <https://reviews.llvm.org/D22837#502775>, @dblaikie wrote:
> 
> > "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."
> >
> > Not sure I understand what you were and were not able to verify/test. Could you explain that more/differently?
> 
> 
> 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.
> 
> 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.
> 
> Could you just test that?

Makes sense to me too.

>  
> 
> What I'm not sure about is whether named metadata will always be handled correctly.
> As far as I can tell, named metadata is pretty much read-only. You can only replace it with a new temporary instance.
> Removed code was attempting to manually update named metadata if it happened to reference (directly or not) any IR that the pass changed.
> 
> 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.
> 
> However, if there's a way for a global node to refer to IR, that may need explicit replacement of such node.
> 
> > 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)
> 
> 
> 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.
> 
> 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.

Agreed.

>  
> 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.
> 
> What I need to know is -- when a regular metadata node is modified, am I guaranteed that:
> a) either modification happend in-place, without replacing the node,
> b) or, if the node is replaced, all metadata nodes referencing it (including named metadata nodes) are updated appropriately.
> 
> 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.

As long as you're using RAUW on the global variables, you shouldn't have anything to worry about.  The MDNodes that reference them will be updated to point at the new global variables (through bitcasts, like other IR).

Note that the MDNodes themselves do not have use lists in general, so they do not know who references them.  Their operands will change without changing their address.

> 
> 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.
> 
> 
> https://reviews.llvm.org/D22837 <https://reviews.llvm.org/D22837>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160802/c9f03b2c/attachment.html>