[mlir][LLVM] Add param attr verifiers

[Dinistro]

Summary

This commit introduces unified parameter attribute verifiers to the LLVM
dialect and removes according checks in the export. As LLVM does not
verify the validity of certain attributes on return values, this commit
unifies the handling of argument and result attributes wherever possible.

Depends on D142212

[gysit]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

3136

Let's take over the original comment that explains that the operation result types may be in a transitioning state.

3182–3183

There may be downstream users for this functionality. I would keep the functionality and change the comment to "Note: The struct parameter attributes are not lowered to LLVM IR." or similar.

3210

nit: unit

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

902

This seems to be dead code?

970

nit: if it fits one line I would inline the getArgAttrDict call into the condition.

mlir/test/Dialect/LLVMIR/arg-res-attrs-invalid.mlir

109 ↗

(On Diff #491396)

nit: @variadic_def -> @void_def

113 ↗

(On Diff #491396)

Starting from here the tests seem to be duplicates?

[Dinistro]

address review comments

[Dinistro]

mlir/test/Dialect/LLVMIR/arg-res-attrs-invalid.mlir

113 ↗

(On Diff #491396)

Tests further up check argument attributes, from here on they check result attributes.

[Dinistro]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

3182–3183

@laszlo-luminous (or @laszlokindrat) you initially pushed the struct_attr for C-wrappers. Do you know of any downstream usage of this functionality? The struct_attr functionality is not an LLVM feature and will thus be thrown away when it's exported to LLVM IR.

[definelicht]

mlir/test/Dialect/LLVMIR/arg-res-attrs-invalid.mlir

3 ↗

(On Diff #491621)

Formulation nit (same for all of these).

8 ↗

(On Diff #491621)

Formulation nit (same for all of these).

38 ↗

(On Diff #491621)

Formulation nit (same for all of these).

48 ↗

(On Diff #491621)

Formulation nit (same for all of these).

83 ↗

(On Diff #491621)

Formulation nit (same for all of these).

[Dinistro]

rename test file and add a comment to indicate the start of the respective test cases.

[Dinistro]

Address additional comments

[gysit]

LGTM!

[Dinistro]

Add checks for invalid result attributes. This checks explicitly looks for otherwise valid argument attributes, as there might be arbitrary attributes attached to a result.

[srcarroll]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

3182–3183

@laszlokindrat is on vacation and doesn't have access, so he asked me to respond since I worked with him on this. It's been a very long time so I don't remember everything, but I remember we made these changes to fix a bug in convert-func-to-llvm=emit-c-wrappers back when the pass used to have the emit-c-wrappers option. Here's the issue I posted about it (can't remember why I'm talking about convert-std-to-llvm at first) https://github.com/llvm/llvm-project/issues/53503. Unfortunately I don't remember anything about struct_attr, but the main issue we fixed was that arg_attrs wasn't being updated correctly. So none of this was specifically intended for downstream uses, but rather a bug fix.

[Dinistro]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

3182–3183

Thanks for the response @srcarroll. In that case the struct_attr was probably introduced with the goal of not dropping any information. Unfortunately, this is not supported by LLVM and is therefore ignored while lowering. As there seems to be no downstream usage, I will probably clean this up once this patch has landed.

can't remember why I'm talking about convert-std-to-llvm at first

The pass was originally named std-to-llvm ;)