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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D135202#3906261 <https://reviews.llvm.org/D135202#3906261>, @jcranmer-intel wrote:

> In addition, this creates a notion of type properties that is used to fix some
> of the issues mentioned above. There is a major unresolved question in this
> update as to where the logic to get the type properties of a target extension
> type should be located. I see 4 options:
>
> 1. Specified during type creation.
> 2. Attach to the module akin to existing identified struct type logic.
> 3. Attach to DataLayout via the datalayout string or something similar.
> 4. Lookup table built-in to LLVM.
>
> [Note that the current implementation, a static function in Type.cpp, is not any
> of these options, but it is most similar to option 4, although I would replace
> that with something tablegen-driven most likely, like existing `Attributes.td`.]
>
> These options are laid out from most to least flexible. While I'm not entirely
> happy with how option 4 requires modification to LLVM to effectively add new
> target extension types, it would make it about as difficult to add a new type as
> it would be to add a new attribute, which seems appropriate.

A hard constraint here is that we need to be able to determine type layout from just the Type and DataLayout, so I don't think that anything involving the Module is possible.

TBH, I think your current approach of just hardcoding the type properties is fine. We can extend this to a more dynamic solution later if needed. It might make sense to compute the properties on construction though and store them in the type (which would then also be the entry-point to a more dynamic solution, by accepting these as arguments). I don't think we need TableGen for this either, as there's no need to generate multiple tables from one base data set.



================
Comment at: llvm/include/llvm-c/Types.h:68
  */
-typedef struct LLVMOpaqueType *LLVMTypeRef;
+typedef struct LLVMTargetExtType *LLVMTypeRef;
 
----------------
This doesn't look right...


================
Comment at: llvm/include/llvm/IR/Type.h:277
         getTypeID() == PointerTyID || getTypeID() == X86_MMXTyID ||
-        getTypeID() == X86_AMXTyID)
+        getTypeID() == X86_AMXTyID || getTypeID() == TargetExtTyID)
       return true;
----------------
Should this check whether the layout type is sized? (I.e., does it make sense to also support unsized target types?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135202