[PATCH] D104827: [DebugInfo] Enforce implicit constraints on `distinct` MDNodes

Fri Jun 25 06:12:55 PDT 2021

StephenTozer accepted this revision.
StephenTozer added a comment.

Good patch, this is a good way of generifying the logic for inline metadata - and improving the documentation at the same time, which is a big plus!

In D104827#2839615 <https://reviews.llvm.org/D104827#2839615>, @scott.linder wrote:

> I'm not sure my overall approach to this patch is actually the best way to
> proceed. The biggest issue is that DIArgList is more particular about where it
> can appear, so most of this generic support for UNIQUED nodes is just dead code
> for DIArgList.

I think this is mostly unavoidable as-is; this patch //does// nicely take care of the inline printing/parsing logic, trying to also simplify the function-local aspects would be out of scope. In that regard, the class that `DIArgList` shares its behaviour with is `ValueAsMetadata`/`LocalAsMetadata`. Unfortunately there is no easy way to generify that logic, as `ValueAsMetadata` is a unique case since it is printed as `i32 %value` rather than `!ClassName(...)`. And I don't think I'd quite call the generic support "dead code" for DIArgList - it may be an error in all cases except when parsed as a function operand, but it still does need to be handled, and if some change in future affects how we print/parse inline metadata then that change must still apply equally to both `DIExpression` and `DIArgList`, which this patch facilitates.

> A potential alternative change is to forbid the non-inline syntax for
> DIExpression entirely, as is done with DIArgList implicitly by requiring
> it appear in the context of a function.

Personally, I could go either way with this. Currently, DIExpression does not actually use function state in any way, but will still only ever be used in the context of a function. In particular, I'm not sure what value there could ever be in using a `DIExpression` in a named metadata node. It seems that in principle it would be better suited to being explicitly function-local metadata, but I'm not sure if it's worth putting in the effort to make the change when doing so doesn't grant us much benefit and also breaks parsing for old IR that contains ID'd `DIExpression`s.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104827