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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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.

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



================
Comment at: llvm/docs/BitCodeFormat.rst:1342
+TYPE_CODE_OPAQUE_TYPE Record
+^^^^^^^^^^^^^^^^^^^^^^^
+
----------------
nit: You need moar `^`


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


================
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.
+
----------------
There should be a mention of the type size and alignment somewhere.


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:758
+  static OpaqueType *get(LLVMContext &Context, StringRef Name,
+                         ArrayRef<Type *> Types, ArrayRef<unsigned> Ints);
+
----------------
You can likely `= None` these and don't need a separate overload.


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


================
Comment at: llvm/lib/IR/Function.cpp:857
+    Result += "o";
+    Result += OTy->getName();
   } else if (Ty) {
----------------
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.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:213
+        return false;
+      return true;
+    }
----------------
`return Name == that.Name && TypeParams == that.TypeParams && IntParams == that.IntParams`?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:1718
+  if (OldTy->isOpaqueTy() || NewTy->isOpaqueTy())
+    return false;
+
----------------
Does `CastInst::isBitCastable` need an adjustment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135202