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

[David Blaikie via llvm-commits]

+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> wrote:

> tra added a comment.
>
> In 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?


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


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