This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] Opaque pointer support for atomic and call ops.
ClosedPublic

Authored by gysit on Jan 30 2023, 5:10 AM.

Details

Summary

This revision adapts the printers and parsers of the LLVM Dialect
AtomicRMWOp, AtomicCmpXchgOp, CallOp, and InvokeOp to support both
opaque and typed pointers by printing the pointer types explicitly.
Previously, the printers and parser of these operations silently assumed
typed pointers. This assumption is problematic if a lowering or the
LLVM IR import produce LLVM Dialect with opaque pointers and the IR is
then printed and parsed, for example, when running mlir-translate. In
LLVM IR itself all tests with typed pointers are already gone. It is
thus important to start switching to opaque pointers.

This revision can be seen as a preparation step for the switch of the
LLVM Dialect to opaque pointers. Once printing and parsing works
seamlessly, all lowerings to LLVM Dialect can be switched to produce
opaque pointers. After a transition period, LLVM Dialect itself can by
simplified to support opaque pointers only.

Diff Detail

Event Timeline

gysit created this revision.Jan 30 2023, 5:10 AM
Herald added a project: Restricted Project. · View Herald Transcript
gysit requested review of this revision.Jan 30 2023, 5:10 AM

LGTM, especially with all the red from the assembly format :-)
I'll leave final approval to someone else however given the impact on syntax.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
1693–1694

tiny nit in wording

Overall, this is looking good to me.
Please wait for the opinions of others as they might have other concerns.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
548

Personally, prefer to just use the functional-type assembly format here: (!llvm.ptr, f32) -> (). Separating the types but only applying the result types to the second one seems a bit odd.

While this would force us to manually construct the functional types, some other parts of the parsing and printing could be simplified.

Any other opinions on that?

mlir/test/Target/LLVMIR/llvmir.mlir
1011–1012

it might make sense to write some of these tests with opaque pointers.

gysit added inline comments.Jan 30 2023, 8:51 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
548

What I like about the current solution is that the function type of the called function is explicit. If we include the pointer in the arguments of the function type it is not directly related to the type of the called function anymore. Additionally, with the current solution the function type remains the same for direct and indirect calls.

I am thus sightly in favor of the current solution. Happy to hear other opinions.

nikic added a subscriber: nikic.Jan 30 2023, 9:22 AM

Do I understand correctly that the additional type argument is needed as an intermediate step to distinguish typed/opaque pointer MLIR right now, but would not be necessary if only opaque pointers were supported?

Do I understand correctly that the additional type argument is needed as an intermediate step to distinguish typed/opaque pointer MLIR right now, but would not be necessary if only opaque pointers were supported?

No since the pointers also have an address space which has to be printed. Unless there was any restriction on indirect call and those atomic ops that they only work in the default address space.

ftynse accepted this revision.Jan 31 2023, 4:26 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
548

Flattening everything into a single function type will look misleading here, especially during the transition period where we still have typed pointers: llvm.call %1(%0) : (!llvm.ptr, f32) -> () is naively read as calling a function that takes a pointer and an f32, given that the previous syntax would mean exactly that. The syntax with separate function type is okay.

This revision is now accepted and ready to land.Jan 31 2023, 4:26 AM
gysit updated this revision to Diff 493606.Jan 31 2023, 7:11 AM
gysit marked 3 inline comments as done.

Address reviewer comments and rebase.

Additionally, fix a problem with the export of indirect calls when using opaque
pointers. Instead of using the LLVM function type of the pointer, we now
recompute the LLVM function type given the argument and result types. The
current solution does not support indirect calls of variadic function, which
is not a regression compared to the old solution. Previously the same
LLVM function type reconstruction happened during parsing. A number of
additional tests tries to increase the test coverage for these corner
cases.

gysit updated this revision to Diff 493611.Jan 31 2023, 7:29 AM

Fix comments.

Dinistro accepted this revision.Jan 31 2023, 11:28 PM

LGTM module the comment.

Could you maybe also add a FIXME for the varargs issues you mentioned in the previous comment?

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

Didn't we discuss that void should never be explicitly used in the IR? Wouldn't it be cleaner to return an error if there is void to stop further misuse?

gysit updated this revision to Diff 493850.Feb 1 2023, 12:17 AM
gysit marked an inline comment as done.
gysit edited the summary of this revision. (Show Details)

Address last review comments.

This revision was automatically updated to reflect the committed changes.