[PATCH] D135202: [IR] Add an opaque type to LLVM.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: llvm/docs/LangRef.rst:3609
+
+Opaque Type
+"""""""""""
----------------
jcranmer-intel wrote:
> nhaehnle wrote:
> > nikic wrote:
> > > Something that's a bit confusing is that we have both "opaque type" and "opaque structure type", the former spelled `%x = type opaque`, the latter spelled `opaque("x")`. This seems rather unfortunate.
> > > 
> > > I think a viable way forward would be to keep the old notion of opaque types, but move away from the "opaque" terminology for them. Effectively, an opaque struct type is just an identified struct type with zero members.
> > Thinking about this a bit more, I think it would be good to move away from the "opaque" naming entirely and instead call these new types something like an "extension" type or a "generic" type.
> > 
> > It's a bit of a philosophical point, but the types being added here aren't necessarily opaque in a universal sense. They are opaque to core IR instructions, but they are not opaque to e.g. intrinsics that are added specifically for working with them.
> > 
> > (And the same is true for the "opaque" structs as well. "Identified struct type with zero members" is what they are, so why not just run with it.)
> "Opaque types" has some precedent (it's what the SPIR-V specification calls these types), but I suspect that terminology derives from opaque types, given the role of LLVM in SPIR's history.
> 
> `%x = type opaque` and `opaque("x")` being similar is indeed very unfortunate. For a while, I've been thinking that opaque pointers themselves make opaque structs obsolete--there's very little you can do on them once a pointer to them is indistinguishable from any other pointer--which would help remove the confusion.
> 
> As far a better name, I'm having a tough time coming up with a short name. Something with "target" in it I think captures the semantics well, but "target type" feels insufficient, and "target-extension type" or "target-specified type" a little wordy. But moving to a `target("spirv.Image", void, 2, 0, 0, 0, 0, 0, 1)` syntax (to use an actual use of this for SPIR-V) I think reads slightly better than using `opaque`.
How about `ext(...)`? But `target(...)` is fine with me, too.


================
Comment at: llvm/lib/IR/Constants.cpp:1092
+  if (isa<OpaqueType>(Ty))
+    return ElementCount::getNull();
   return ElementCount::getFixed(Ty->getStructNumElements());
----------------
jcranmer-intel wrote:
> nikic wrote:
> > This looks pretty dubious. We probably shouldn't be modelling opaque types using ConstantAggregateZero, as it is not an aggregate type.
> > 
> > Is zeroinitializer support actually required?
> Yes, for at least some of the types. (https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpConstantNull lists the opaque types for which something like null exists).
> 
> I agree that `ConstantAggregateZero` is somewhat dubious as a class, but it seemed more fitting than any other class, not least of which being the fact that it causes `zeroinitializer` to work without any extra effort.
> 
> I'm open to suggestions as to what to name a new zeroinitializer class for opaque pointers--I don't have any great names off my head.
Would an intrinsic that returns the null value work? Or are null initializers also allowed in global variables?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135202