Page MenuHomePhabricator

[mlir][LLVM] Add param attr verifiers
ClosedPublic

Authored by Dinistro on Mon, Jan 23, 8:33 AM.

Details

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

Diff Detail

Event Timeline

Dinistro created this revision.Mon, Jan 23, 8:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
Dinistro requested review of this revision.Mon, Jan 23, 8:33 AM
Dinistro edited the summary of this revision. (Show Details)Mon, Jan 23, 8:37 AM
gysit added inline comments.Mon, Jan 23, 1:14 PM
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 updated this revision to Diff 491621.Mon, Jan 23, 11:58 PM
Dinistro marked 7 inline comments as done.

address review comments

Dinistro added inline comments.Mon, Jan 23, 11:59 PM
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 added inline comments.Tue, Jan 24, 12:11 AM
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 added inline comments.
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 updated this revision to Diff 491630.Tue, Jan 24, 12:26 AM

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

Dinistro updated this revision to Diff 491638.Tue, Jan 24, 12:37 AM
Dinistro marked 4 inline comments as done.

Address additional comments

Dinistro marked an inline comment as done.Tue, Jan 24, 12:37 AM
Dinistro marked an inline comment as not done.Tue, Jan 24, 1:16 AM
gysit accepted this revision.Tue, Jan 24, 2:23 AM

LGTM!

This revision is now accepted and ready to land.Tue, Jan 24, 2:23 AM
Dinistro updated this revision to Diff 491716.Tue, Jan 24, 4:47 AM

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 added inline comments.
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 added inline comments.Wed, Jan 25, 4:54 AM
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 ;)

This revision was automatically updated to reflect the committed changes.