[flang-commits] [flang] [mlir] [MLIR][LLVMIR][DLTI] Add #llvm.target, #llvm.data_layout and TargetAttrInterface (PR #145899)

[Fabian Mora via flang-commits]

fabianmcg wrote:

> 1. There's a circular dependency between `LLVMInterfaces` and `LLVMAttrs` in that the new `TargetAttrInterface` requires `LLVMTargetFeaturesAttr` (for the `features` of a target) and there are attrs in `LLVMAttrs` that require interfaces from `LLVMInterfaces`. I break the dependency by moving `LLVMTargetFeaturesAttr` to its own file and having a dedicated CMake-rule to generate `LLVMOpsAttrDefs.h.inc` by way of `LLVMAttrAndEnumDefs.td`

I'd argue is preferable to create `TargetAttrInterfece.td` to break the dependency. That way other dialects wanting to implement the interface don't have to take `LLVMInterfaces` as a dep.


> 2\. For some reason, the parser generated for `` let assemblyFormat = [{`<` `triple` `=` $triple (`,` `chip` `=` $chip^)? (`,` qualified($features)^)? `>`}]; `` does not actually make the `chip` clause optional. That is, it greedily assumes that as it saw `,` from the `chip` clause it is now committed to it. For example, `#llvm.target<triple = "x86_64-unknown-linux", #llvm.target_features<["+mmx", "+sse"]>>` yields an `expected 'chip'` parser error.

I'd recommend using the struct directive, that way the elements can appear in any order, and should handle the optional elements correctly.

https://mlir.llvm.org/docs/DefiningDialects/AttributesAndTypes/#struct-directive

If I recall correctly, the above issue is caused because the parser generated MLIR uses only one lookahead token, so the parser doesn't know whether it's parsing chip or features because they both start with a `,`.

Also, I'm not sure whether making `chip` optional is a good idea.



> 3\. How do I properly hang a `unique_ptr<llvm::TargetMachine>` of off `TargetAttr`? The current bare pointer works but probably is not safe, e.g. in case the context gets deleted. See discussion with @rengolin above.

I imagine you want this for caching the TM. But why do you want this? When is it going to be reused? I think we should use to import the DL for now.

> 6\. Test cases should be made aware of which backends the compiled `llvm-project` has available and be skipped if not applicable. CI is happy for now, which is actually wrong.

See this:
https://github.com/llvm/llvm-project/blob/main/mlir/test/Dialect/GPU/module-to-binary-nvvm.mlir#L1


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