[Mlir-commits] [mlir] Reimplementing target description concept using DLTI attribute (PR #92138)

[Fabian Mora]

fabianmcg wrote:

Thanks for the replies @rengolin! Now I understand a little better your rationale. Btw, none of my comments are blocking I think it's a good implementation, so if you already have consensus go ahead.

The summary of my point is: the generic attributes and interfaces could be simpler and in consequence more generic, leaving all the real burden to derived attributes:

> It would be wrong to come up with a fixed _"generic"_ design right now, trying to predict all cases, and having no one actually using any of those. It's much simpler and faster to agree on a base design and improve from there, with the input of all users, not a single one.

I 100% agree, that's why I think we shouldn't include the `getSpecForMaxVectorOpWidth`, `getSpecForL1CacheSizeInBytes` in the interface nor in the generic attribute. Those belong in derived target attributes.

> I believe we need both extensions. Both are directly derivative of this PR and can be extended independently, so we really should not include it in this PR. Once we agree on the base structure, we'll continue with the more advanced strategies.

I think another solution for this is consolidating the `TargetDeviceSpecInterface` and `TargetSystemSpecInterface` in a single `TargetSpecInterface`.  And having something similar to:
```
struct Target {
  TargetList subTargets;
  PropertyMap properties;
};
```
Or even making properties include `subTargets`.

In this case, the interface only has `getSpecProp(StringRef)`, `getDeviceList()`, which would cover all target description cases without requiring extra changes in future PRs. Making things like: 
```mlir
// Using generic target attribute:
#Node = #dlti.target</*subTargets=*/[
  #rocdl.target<chip="gfx90a">,
  // Generic attribute:
  #dlti.target</*properties=*/{kind = "CPU", model = "4750U", vendor = "AMD"}>
  ], {topology = #numa.topology<...>}>
// Derived target attribute:
#cluster.target<
  node = #Node,
  num_nodes = 4096,
  interconnect = #fabric.infiniband<NDR>
>
```
Implementable after this PR.

> It's a different design (means we need to sort maps before printing them out) but equally unique.

This could create issues because nothing prevents creating two attributes with the same unordered device list but with a different list order (ie. [dev[id=0], dev[id=1]], [dev[id=1], dev[id=0]]). Those attributes wouldn't be equal at the time of creation, but equal after printing and parsing.

If we want to impose an order on the devices or provide identifiers, I still don't see how adding ID's to the targets is needed or improves the design. 
For this I think it's better doing a `DictAttr`, as the correspondence between ID and device is unambiguous for the developer and in the IR.

 

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