This is an archive of the discontinued LLVM Phabricator instance.

[mlir] initial support for opaque pointers in the LLVM dialect
ClosedPublic

Authored by ftynse on Apr 7 2022, 7:24 AM.

Details

Summary

LLVM IR has introduced and is moving forward with the concept of opaque
pointers, i.e. pointer types that are not carrying around the pointee type.
Instead, memory-related operations indicate the type of the data being accessed
through the opaque pointer. Introduce the initial support for opaque pointers
in the LLVM dialect:

  • LLVMPointerType to support omitting the element type;
  • alloca/load/store/gep to support opaque pointers in their operands and results; this requires alloca and gep to store the element type as an attribute;
  • memory-related intrinsics to support opaque pointers in their operands;
  • translation to LLVM IR for the ops above is no longer using methods deprecated in LLVM API due to the introduction of opaque pointers.

Unlike LLVM IR, MLIR can afford to support both opaque and non-opaque pointers
at the same time and simplify the transition. Translation to LLVM IR of MLIR
that involves opaque pointers requires the LLVMContext to be configured to
always use opaque pointers.

Diff Detail

Event Timeline

ftynse created this revision.Apr 7 2022, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 7:24 AM
ftynse requested review of this revision.Apr 7 2022, 7:24 AM
ftynse updated this revision to Diff 421225.Apr 7 2022, 8:24 AM

git add tests

nikic added a comment.Apr 12 2022, 7:11 AM

Thanks for working on this! Not really familiar with MLIR, so can't do much useful review here.

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

$ptr_type seems a bit confusing here, maybe $elem_type?

348

Do we need to look through vectors here?

Or actually, should this be using the new getSourceElementType() method?

mlir/test/Dialect/LLVMIR/opaque-ptr.mlir
5

Unnecessary %arg1 argument.

ftynse updated this revision to Diff 422460.Apr 13 2022, 3:56 AM
ftynse marked 3 inline comments as done.

Address review.

Thanks for working on this! Not really familiar with MLIR, so can't do much useful review here.

If the use of opaque pointers looks reasonable to you, this is all I need.

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

Good catch, thanks! Forgot that I added it.

wsmoses added inline comments.Apr 13 2022, 8:26 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
303

Perhaps it's better to do the ternary before the getElementType? While this works for now, if for some reason opaque pointers mean LLVMPointerType isn't guaranteed to have an element type, this may error?

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

Should this not also check that the pointertype is not opaque?

Edit: Ah nevermind, I see that the convention is that the first argument is the value type, if there are two types.

I do think there's a bug here though -- namely what if the value you're storing is itself a pointertype. You can never hit the else.

ftynse updated this revision to Diff 422537.Apr 13 2022, 9:29 AM
ftynse marked an inline comment as done.

Address review.

wsmoses accepted this revision.Apr 13 2022, 9:35 AM
This revision is now accepted and ready to land.Apr 13 2022, 9:35 AM
ftynse added inline comments.Apr 13 2022, 9:41 AM
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
792

Very nice catch, thanks!

ftynse marked an inline comment as done.Apr 13 2022, 9:41 AM
This revision was landed with ongoing or failed builds.Apr 14 2022, 4:23 AM
This revision was automatically updated to reflect the committed changes.