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

[Rolf Morel via flang-commits]

rolfmorel wrote:

@fabianmcg thanks for pointing that out! That should resolve bullet 4 up above I believe.

Any other feedback on this PR would be much appreciated. In particular any help with addressing the remaining bullets:

> 1. As the `LLVMTargetFeaturesAttr` attribute already exists we have the following:
>    
>    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`.
>       
>       1. Is there a better way of dealing with this circular dependency?
>    2. Currently, `#nvvm.target` and `#rocdl.target` take `features` as a `StringAttr`. Seems to me they should take `LLVMTargetFeaturesAttr` which would make them eligible for/compatible with implementing `TargetAttrInterface`. I have not spent time on transitioning them yet.
> 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.
>    
>    1. What's the best solution here? Just give in and write a custom parser?
> 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.
> 
> 4. ~Had to move the `DataLayoutImporter` from `lib/Target/LLVMIR` to `Dialect/LLVMIR/IR`, in support of `#llvm.data_layout` being a wrapper around the corresponding `#dlti.dl_spec`, which causes a linking dependency on the DLTI-dialect.~
> 
> 5. What is the strategy around verification? See discussion with @rengolin above.
> 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.



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