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

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel added a comment.

Sorry for the delay in responding to comments, I've been trying to get the subsequent changes that use for SPIR-V polished.

In D135202#3836252 <https://reviews.llvm.org/D135202#3836252>, @nikic wrote:

> The implementation looks reasonable. In terms of design, I basically have the same concern as everyone else: Hardcoding the layout to `ptr` seems pretty dubious. The suggestion to specify the layout type as a parameter seems pretty sensible.

As pointed out, specifying an underlying type isn't necessarily feasible. But I could add optional type properties that would include size and alignment to start with (and have those default to `sizeof(ptr)` and size, respectively)--how does that sound?

> Less importantly, I wonder whether the integer parameters should be represented as APInt rather than 32-bit ints.

I've considered this as well.



================
Comment at: llvm/docs/LangRef.rst:3609
+
+Opaque Type
+"""""""""""
----------------
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`.


================
Comment at: llvm/docs/LangRef.rst:3634
+of name and parameters are defined by the target. When being defined in LLVM IR,
+all of the type parameters must precede all of the integer parameters.
+
----------------
nikic wrote:
> There should be a mention of the type size and alignment somewhere.
Noted, will fix.


================
Comment at: llvm/lib/IR/Constants.cpp:1092
+  if (isa<OpaqueType>(Ty))
+    return ElementCount::getNull();
   return ElementCount::getFixed(Ty->getStructNumElements());
----------------
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.


================
Comment at: llvm/lib/IR/Function.cpp:857
+    Result += "o";
+    Result += OTy->getName();
   } else if (Ty) {
----------------
nikic wrote:
> This must be unique per type, so you need to include all the nested information as well. You can add tests using the ssa.copy intrinsic.
Good catch--I had originally coded this up without any type/integer parameters, and missed the need to change this location as well. Will fix!


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:1718
+  if (OldTy->isOpaqueTy() || NewTy->isOpaqueTy())
+    return false;
+
----------------
nikic wrote:
> Does `CastInst::isBitCastable` need an adjustment?
It turns out not to need it--`CastInst::isBitCastable` returns false if `Type::getPrimitiveSizeInBits` returns 0 for either parameter, and `Type::getPrimitiveSizeInBits` returns 0 for opaque types.

(This made it easy to confirm when optimizations caused problems--they tried to create bitcast instructions with opaque types, and that failed.) I can add an extra check for safety.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135202