[PATCH] D147697: [IR] Add TargetExtTypeClass

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

nhaehnle added a comment.

It seems this comes down to this question: Should the type properties be per type name or should they be per type?

But so far, it seems that nobody has very strong feelings in either direction. Personally, I'd err on the side of flexibility (hence the current shape of this change), but I don't feel strongly about it. How do we decide such things? :)



================
Comment at: llvm/docs/LangRef.rst:3837
+        hasZeroInit: i1 true,
+      }
 
----------------
jcranmer-intel wrote:
> nikic wrote:
> > nhaehnle wrote:
> > > nikic wrote:
> > > > This could use some clarification on how the properties for target types with different arguments relate, if at all. If you define properties for `target("mytype")`, do those also apply to `target("mytype", i32)`?
> > > > 
> > > > Do you need to repeat the properties for every combination of arguments in the IR, or is there some inheritance?
> > > They don't relate and there is no inheritance, every target type is distinct and has its own properties. I did briefly consider the possibility of inheritance, but it seemed to quickly get very complex and not that useful. I'm going to try to clarify that.
> > I see. I think this point could use some input from people who are using target types, in particular @jcranmer-intel.
> > 
> > The way I originally expected this to work is that the target type properties are independent of the type arguments entirely, so that `target("mytype")` and all `target("mytype", Ti, Ni)` share the same properties.
> > 
> > I'm not familiar enough with how these types are used to say whether independent properties for each argument combination makes sense or not.
> All of the use cases I have had have the property that properties are based entirely on the string property. Indeed, one property that could be useful to check is the number of integer/type arguments!
> 
> That said, there are some cases where varying type parameters might change the properties of the type--suppose something like `target("complex", float)` versus `target("complex", double)` or `target("decfp", 64)` versus `target("decfp", 32)`. Although for many of those types, I do question if they should be target extension types in the first place.
There's a question of how strongly you feel about this and how it would affect the runtime representation.

If we did change the properties to be per type name instead of per type, we should probably also store them per type name.


================
Comment at: llvm/include/llvm/IR/TargetExtType.h:51-59
+  /// Given a partially initialized type, return the layout type (defaults to
+  /// void, indicating that the type can't be laid out in memory).
+  using GetLayoutTypeFn = Type *(TargetExtType *T);
+  GetLayoutTypeFn *GetLayoutType = nullptr;
+
+  /// Given a partially initialized type, return the type properties (defaults
+  /// to 0).
----------------
jcranmer-intel wrote:
> I question the need for these to be function pointers rather than instance properties. (The verifier function does need to be its own function).
This is basically the same question as before. If we decide that these properties should be fixed on a per-type-name basis, then yes, we can just make them member variables here. Otherwise, they need to be fallbacks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147697