This is an archive of the discontinued LLVM Phabricator instance.

Add opaque pointers to the llvm-c API
ClosedPublic

Authored by nico-abram on May 7 2022, 11:42 PM.

Details

Summary

This is based on https://reviews.llvm.org/D125168 which adds a wrapper to allow use of opaque pointers from the C API.

I added an opaque pointer mode test to echo.ll , and to fix assertions that forbid the use of mixed typed and opaque pointers that were triggering in it I had to also add wrappers for setOpaquePointers and supportsTypedPointers.

I also changed echo.ll to remove a bitcast i32* %x to i8*, because passing it through llvm-as and llvm-dis was generating a %0 = bitcast ptr %x to ptr, but when building that same bitcast in echo.cpp it was getting elided by IRBuilderBase::CreateCast (https://github.com/llvm/llvm-project/blob/08ac66124874d70dab63c731da0244f9e29ef168/llvm/include/llvm/IR/IRBuilder.h#L1998-L1999).

cc @lerno

Diff Detail

Event Timeline

nico-abram created this revision.May 7 2022, 11:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 11:42 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
nico-abram requested review of this revision.May 7 2022, 11:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 11:42 PM
nikic added a subscriber: nikic.May 8 2022, 1:18 AM
nikic added inline comments.
llvm/include/llvm-c/Core.h
1453

nit: Missing period at end.

llvm/tools/llvm-c-test/echo.cpp
1004

You can use this unconditionally.

1037

Same here.

1395

Should be OpaquePointers for consistency?

Also, can we just change llvm_echo to accept the argument rather than adding two wrappers?

nico-abram updated this revision to Diff 427919.May 8 2022, 2:26 AM

Addressed @nikic's comments, thanks for the feedback.

nikic added inline comments.May 9 2022, 2:18 AM
llvm/include/llvm-c/Core.h
556

I'm a bit uncertain about this API. The supportsTypedPointers() naming dates back to a time where we supported mixed typed and opaque pointers, so "supports opaque pointers" and "not supports typed pointers" were not equivalent. There's a couple of different possibilities here:

  • LLVMContextSupportsTypedPointers() as proposed.
  • LLVMContextHasOpaquePointers() the inverse, which mirrors LLVMContextSetOpaquePointers() a bit more nicely.
  • Expose LLVMPointerTypeIsOpaque() or similar instead, which would work on a pointer type rather than context.

The latter is slightly less general (you can only use it if you already have a pointer type), but it's the one that we actually use most often in practice. We only use supportsTypedPointers() for automatic switching into opaque pointer mode during LL/BC parsing.

I'd personally lean towards exposing LLVMPointerTypeIsOpaque().

llvm/tools/llvm-c-test/echo.cpp
1034

nit: Combine declaration and assignment.

llvm/tools/llvm-c-test/main.c
52

Make it --echo --opaque-pointers for the sake of consistency?

nico-abram added inline comments.May 10 2022, 3:27 PM
llvm/tools/llvm-c-test/main.c
52

I used -opaque-pointers since that's what the tools use, like llvm-as and llvm-dis, as documented here

If keeping it consistent with the rest of llvm-c-test is more important I can change it

nikic added inline comments.May 13 2022, 7:51 AM
llvm/tools/llvm-c-test/main.c
52

The preferred option format is tool-specific -- opt etc do accept options in --opaque-pointers format as well, but for historical reason we tend to use the single-dash spelling for them.

aeubanks added inline comments.
llvm/include/llvm-c/Core.h
556

+1

Thanks for the feedback.

I uploaded a new diff, removed LLVMContextSupportsTypedPointers and added LLVMPointerTypeIsOpaque, also changed the echo.cpp flag to use the -- style.

nico-abram marked 9 inline comments as done.May 13 2022, 5:12 PM
nikic accepted this revision.May 14 2022, 1:28 AM

LGTM

This revision is now accepted and ready to land.May 14 2022, 1:28 AM

I don't have commit rights, could somebody please land this for me?

This revision was landed with ongoing or failed builds.May 16 2022, 1:53 AM
This revision was automatically updated to reflect the committed changes.