[Mlir-commits] [mlir] [mlir] Add support for recursive elements in DICompositeTypeAttr. (PR #74948)

[Markus Böck]

zero9178 wrote:

> Thanks for tackling this problem!
> 
> I have two initial comments:
> 
> 1) This problem looks like an interesting use case for https://github.com/llvm/llvm-project/pull/66663. With that PR, it may be possible to solve the printing and parsing without writing a lot of code by hand, which we should avoid if possible. The idea of the pr is to support attribute alias printing in the presence of cyclic dependencies. 
> 
> 2) I think it may be worth considering `DistinctAttr` here instead of a string identifier (https://reviews.llvm.org/D153360). The distinct attribute was developed to model distinct metadata nodes in MLIR. You can create distinct attributes in parallel code and they are then numbered in a deterministic way during printing and parsing similar to SSA values. It makes especially sense since AFAIK the identifier of a DICompositeType is not necessarily sufficient for uniquing. There can be multiple DICompositeType metadata nodes with the same identifier (the definition and possibly multiple forward declarations). At least this is my interpretation of the language reference (https://llvm.org/docs/LangRef.html#dicompositetype). Also is there a guarantee that a recursive DICompositeType has an identifier?
> 
> I am not sure how easy it is to land 1) but it may be worth considering to revive the discussion (either on the PR or maybe better on discourse (https://discourse.llvm.org/t/rfc-supporting-aliases-in-cyclic-types-and-attributes/73236).

Regarding 1), just landing those PRs won't improve the parsing and printing code in its current state. This PR already nicely uses the APIs for doing cyclic printing that the alias statement infrastructure relies on, meaning no changes in the code of this PR would be required after the alias support would land.

Improving the state of mutable attributes and types by adding support for ODS will require further work ontop of what is proposed in the RFC.

Regarding implementing the RFC, I likely won't get to pushing on it until February. While I think the approach should be fine, there are implementation quality issues in the current PRs. 

I would therefore suggest to keep the approach here as is.  

https://github.com/llvm/llvm-project/pull/74948