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

[Justin Wilson]

waj334 wrote:

> Thanks for tackling this problem!
> 
> I have two initial comments:
> 
> 1. This problem looks like an interesting use case for [[mlir] Add support for parsing and printing cyclic aliases #66663](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.

For now I'll move on without #1 and I'll refactor if it lands between now and when this is in an acceptable state. As for printing and  parsing, the only strange quirk I found was with elements needing to appear last in the comma separated list because I was trying to keep most of the old IR format while also making the other attributes "immutable". A side-effect would be that if any other attribute appears after elements, they would not get set because they need to be constructed with the id initially. I think I'm going to separate the elements from the rest of the attributes to make it more straightforward to parse. I'm thinking:

llvm.di_composite_type<distinct[0]<>,(elements...) other = ..., attr = ..., params = ... >

where of course the id parameter can be omitted.

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

This is a reasonable (and easy) change. I'll replace the string id.

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