-
Notifications
You must be signed in to change notification settings - Fork 156
[CIR] Add special type for vtables #1745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This change introduces a new type, cir.vtable, to be used with operations that return a pointer to a class' vtable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments – potentially worth clarifying in documentation, naming, or as future improvements:
-
Is this type intended only for top-level vtable manipulation, i.e., not for representing specific vtable elements? It's currently unclear whether pointer arithmetic is meant to be allowed here.
If the intention is to also represent individual vtable elements (e.g., a pointer to a function pointer), a name like
!cir.vtable_element
(or similar) might be more descriptive. As it stands,!cir.ptr<!cir.vtable>
suggests that another level of indirection or some additional operation is required to access the actual function pointer.On the other hand, I dislike use of
!cir.vtable_element
in cases likedynamic_cast
, where a different abstraction may be more appropriate.Perhaps it would make sense to introduce two distinct types:
!cir.vtable_ptr
(instead of!cir.ptr<!cir.vtable>
) to disallow arbitrary pointer arithmetic on vtables.- A separate type to represent specific vtable elements.
I’m basing this feedback primarily on what’s visible in this PR—I haven’t done a deep dive into where and how vtables are intended to be used across the broader system. Nonetheless, these use cases might be worth clarifying in the type documentation.
-
It might be worth considering the addition of dedicated vtable-related operations in the future, to avoid relying on long chains of casts, loads, and stores?
-
How does this representation relate to
cir::MethodType
? Some clarification around this would be helpful, especially in terms of how method types are expected to interact with vtables. But this is probably for further discussion as method types feels half baked.
Also, I’ve been reviewing how other high-level IRs handle similar concepts. One worth examining is Swift SIL.
def CIR_AnyVTableType : CIR_TypeBase<"::cir::VTableType", | ||
"vtable type">; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def CIR_AnyVTableType : CIR_TypeBase<"::cir::VTableType", | |
"vtable type">; | |
def CIR_AnyVTableType : CIR_TypeBase<"::cir::VTableType", "vtable type">; |
def CIR_VTableType : CIR_Type<"VTable", "vtable", | ||
[DeclareTypeInterfaceMethods<DataLayoutTypeInterface>]> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def CIR_VTableType : CIR_Type<"VTable", "vtable", | |
[DeclareTypeInterfaceMethods<DataLayoutTypeInterface>]> { | |
def CIR_VTableType : CIR_Type<"VTable", "vtable", [ | |
DeclareTypeInterfaceMethods<DataLayoutTypeInterface> | |
]> { |
@xlauko Thanks for the review! It does seem as though I've greatly oversimplified this. I'm going to put it aside for now, because it does need some more thought. I'll definitely add more documentation when I get back to it. |
This change introduces a new type, cir.vtable, to be used with operations that return a pointer to a class' vtable.