Skip to content

[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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andykaylor
Copy link
Collaborator

This change introduces a new type, cir.vtable, to be used with operations that return a pointer to a class' vtable.

This change introduces a new type, cir.vtable, to be used with operations
that return a pointer to a class' vtable.
Copy link
Collaborator

@xlauko xlauko left a 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 like dynamic_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.

Comment on lines +270 to +272
def CIR_AnyVTableType : CIR_TypeBase<"::cir::VTableType",
"vtable type">;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def CIR_AnyVTableType : CIR_TypeBase<"::cir::VTableType",
"vtable type">;
def CIR_AnyVTableType : CIR_TypeBase<"::cir::VTableType", "vtable type">;

Comment on lines +350 to +352
def CIR_VTableType : CIR_Type<"VTable", "vtable",
[DeclareTypeInterfaceMethods<DataLayoutTypeInterface>]> {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def CIR_VTableType : CIR_Type<"VTable", "vtable",
[DeclareTypeInterfaceMethods<DataLayoutTypeInterface>]> {
def CIR_VTableType : CIR_Type<"VTable", "vtable", [
DeclareTypeInterfaceMethods<DataLayoutTypeInterface>
]> {

@andykaylor
Copy link
Collaborator Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants