[PATCH] D101704: [IR] Introduce the opaque pointer type

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added reviewers: t.p.northover, dexonsmith.
dexonsmith added a subscriber: t.p.northover.
dexonsmith added a comment.

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.)



================
Comment at: llvm/docs/LangRef.rst:3312
+not have a pointee type. Rather, instructions interacting through pointers
+typically specify the type of the underlying memory they are interacting with.
+
----------------
Is the word "typically" needed here?



================
Comment at: llvm/docs/LangRef.rst:3318
 
       <type> *
 
----------------
Please add another line:

    ptr


================
Comment at: llvm/test/Assembler/opaque-ptr.ll:1
+; RUN: llvm-as < %s | llvm-dis | FileCheck %s
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101704/new/

https://reviews.llvm.org/D101704