Page MenuHomePhabricator

[IR] Introduce the opaque pointer type
ClosedPublic

Authored by aeubanks on May 1 2021, 7:29 PM.

Details

Summary

The opaque pointer type is essentially just a normal pointer type with a
null pointee type.

This also adds support for the opaque pointer type to the bitcode
reader/writer, as well as to textual IR.

To avoid confusion with existing pointer types, we disallow creating a
pointer to an opaque pointer.

Opaque pointer types should not be widely used at this point since many
parts of LLVM still do not support them. The next steps are to add some
very simple use cases of opaque pointers to make sure they work, then
start pretending that all pointers are opaque pointers and see what
breaks.

https://lists.llvm.org/pipermail/llvm-dev/2021-May/150359.html

Diff Detail

Event Timeline

aeubanks created this revision.May 1 2021, 7:29 PM
aeubanks requested review of this revision.May 1 2021, 7:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2021, 7:29 PM

(RFC incoming)

aeubanks updated this revision to Diff 342605.May 3 2021, 5:34 PM

tighten check on num records

Matt added a subscriber: Matt.May 4 2021, 1:07 AM

(feel free to add any other reviewers)

To avoid confusion with existing pointer types, we disallow creating a pointer to an opaque pointer.

Is there some test coverage for this? (I guess some invalid textual and/or bitcode IR that demonstrates how this is checked/failed)

arsenm added a subscriber: arsenm.May 10 2021, 3:40 PM
arsenm added inline comments.
llvm/docs/LangRef.rst
3335

in address space 0

llvm/include/llvm/IR/DerivedTypes.h
658

LLVM does not have a generic address space concept. This is the default address space

aeubanks updated this revision to Diff 344224.May 10 2021, 3:45 PM

rebase
add test for pointer to opaque pointer
s/generic address space/default address space

dblaikie accepted this revision.May 10 2021, 4:04 PM

Looks good to me - though might be worth waiting at least a few days to check if anyone else has feedback. (my approval isn't intended to stifle feedback - folks feel free to chime in!)

This revision is now accepted and ready to land.May 10 2021, 4:04 PM
dexonsmith added a subscriber: t.p.northover.

The code LGTM too, just a couple of nits on docs and tests. Not sure if @t.p.northover has time to look right now, but I added him in case he does. I'm excited to see this moving along!

What's missing for me is where we are in the high-level arc. Why land this now? What's left to do after this lands? What should LLVM developers (and clients) be using the opaque type for once this patch lands (as opposed to the fullness of time)? What should they use typed pointers for?

IMO, adding some of that context to the commit message is important, even if it's light on details / high-level. It might also be useful (not sure...) to expand on it slightly in LangRef as well (some sort of guidance on when opaque vs. typed pointers are appropriate, which can be updated when the guidance changes / as typed pointers get eliminated).

It'd also be nice to have a clear message targeted at LLVM clients added to llvm/docs/ReleaseNotes.rst (which can be updated as guidance changes).

On a related note, plucked from the RFC thread:

Somebody pointed out to me that there's very little actual documentation on opaque pointer types. I'll try to write up some documentation so that the motivation and tradeoffs can be better discussed.

Were you planning to do that in this patch, or as a follow up? (Personally, I don't think it's necessary as a blocker for landing this patch; although pointing to it from the commit message / from the opaque pointer description in LangRef could be a tidy way to resolve my concerns about missing context / high-level arc / etc.)

llvm/docs/LangRef.rst
3312

Is the word "typically" needed here?

3319–3320

Please add another line:

ptr
llvm/test/Assembler/opaque-ptr.ll
2

It'd be nice to make this a double-round trip (pass it through as and dis again before heading to FileCheck), confirming that the textual IR generated by llvm-dis can also be assembled.

Also, at some point I added a RUN line calling verify-uselistorder to all tests in this directory; that's probably bit-rotted (I bet this wouldn't be the only one missing that line) but perhaps you can add one here.

pcc accepted this revision.May 11 2021, 12:03 AM
pcc added a subscriber: pcc.

This looks like a reasonable starting point for switching to opaque pointers in LLVM.

I think the most intuitive next step would be to get things working end-to-end with opaque pointers, starting with simple programs and going from there. As a side effect of that process we will no doubt cause some of the lit tests to start passing with opaque pointers -- as we do so, they can be converted incrementally to use opaque pointers.

Specifically what I would suggest is:

  1. Add a command line flag that makes it so that PointerType::get(Type *, unsigned) always returns an opaque pointer type.
  1. Turn it on and compile hello world with clang -O0. At this point this will almost certainly fail. Fix any issues found that way until hello world compiles and runs.
  1. Repeat for hello world at -O2.
  1. Once hello world is working, move on to larger programs, e.g. the LLVM test suite. As programs start working, have the test suite pass the flag for those programs to prevent backsliding.
  1. In parallel with 2-4, gradually convert the lit tests to use opaque pointers.
aeubanks updated this revision to Diff 344519.May 11 2021, 11:46 AM

update test
add to ReleaseNotes
mention that opaque pointer types aren't ready for general use yet

aeubanks edited the summary of this revision. (Show Details)May 11 2021, 11:48 AM
dexonsmith accepted this revision.May 11 2021, 12:57 PM

update test
add to ReleaseNotes
mention that opaque pointer types aren't ready for general use yet

LGTM!

I think the most intuitive next step would be to get things working end-to-end with opaque pointers, starting with simple programs and going from there. As a side effect of that process we will no doubt cause some of the lit tests to start passing with opaque pointers -- as we do so, they can be converted incrementally to use opaque pointers.

Yeah, this approach sounds pretty reasonable.

I'm curious, have we exhausted what we can do statically? I wonder if refactoring to remove all callers of PointerType::getElementType() (and deleting the API) would be possible as an initial step. Might be simplify the problem in some ways.

I was in the process of auditing callers of PointerType::getElementType() and Type::getPointerElementType() to try and fix them up. That would be a good step.

As for documentation, I'm currently writing that up separately. I'll hopefully have something fairly soon.

This revision was landed with ongoing or failed builds.May 13 2021, 3:23 PM
This revision was automatically updated to reflect the committed changes.