This is an archive of the discontinued LLVM Phabricator instance.

Reland "[mlir][LLVM] Add all LLVM parameter attributes"
ClosedPublic

Authored by Dinistro on Jan 26 2023, 7:55 AM.

Details

Summary

This change was reverted because it introduced a linking issue due to
duplicated symbols. Making sure that the detail helper only has a static
header implementation fixes this issue.

Diff Detail

Event Timeline

Dinistro created this revision.Jan 26 2023, 7:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
Dinistro requested review of this revision.Jan 26 2023, 7:55 AM

I think the commit message should say "excluding the swift specific one" or similar?

mlir/include/mlir/Dialect/LLVMIR/LLVMOpBase.td
66

nit: TODOs are usually commented with rather than / and
nit: typo Restict

70

If TODO and comment only refer to that one line then we may want to move it after the others?

mlir/test/Dialect/LLVMIR/parameter-attrs-invalid.mlir
125

should this be a dereferenceable_or_null test?

Is importing and exporting immarg really required? IIRC this attribute is only valid on arguments of intrinsics, and so far we've been modelling (most of, but should be all of) these specific intrinsics to have attributes for immarg arguments within the LLVM Dialect.

@zero9178 thanks for that hint. That makes perfect sense!

Is importing and exporting immarg really required? IIRC this attribute is only valid on arguments of intrinsics, and so far we've been modelling (most of, but should be all of) these specific intrinsics to have attributes for immarg arguments within the LLVM Dialect.

I didn't realise this, thanks for pointing it out. In that case, we can drop the import of immarg entirely. I assume that the same applies to elementtype, as the doc states they are only valid on intrinsics.

Dinistro updated this revision to Diff 492656.Jan 27 2023, 12:18 AM
Dinistro marked 3 inline comments as done.

address review comments and drop intrinsic specific attributes.

gysit accepted this revision.Jan 27 2023, 12:28 AM

LGTM!

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
912

nit: Let's spell out the type here and above.

This revision is now accepted and ready to land.Jan 27 2023, 12:28 AM
Dinistro updated this revision to Diff 492658.Jan 27 2023, 12:34 AM
Dinistro marked an inline comment as done.
Dinistro edited the summary of this revision. (Show Details)

Commit message change and additional comment addressing.

Dinistro edited the summary of this revision. (Show Details)Jan 27 2023, 12:35 AM
This revision was automatically updated to reflect the committed changes.
Dinistro reopened this revision.Jan 30 2023, 1:03 AM
This revision is now accepted and ready to land.Jan 30 2023, 1:03 AM
Dinistro updated this revision to Diff 493232.Jan 30 2023, 1:15 AM

Fix the broken build by making the detail helpers static header functions.

Dinistro retitled this revision from [mlir][LLVM] Add all LLVM parameter attributes to Reland "[mlir][LLVM] Add all LLVM parameter attributes".Jan 30 2023, 1:19 AM
Dinistro edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.