-
Notifications
You must be signed in to change notification settings - Fork 159
[CIR] Add special type and new operations for vptrs #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
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. |
d2c4ab8
to
8f89224
Compare
This change introduces a new type, cir.vtable, to be used with operations that return a pointer to a class' vtable.
a0aca63
to
0be2244
Compare
@tommymcm I couldn't add you as a reviewer, but I'd like to hear your thoughts on this change. |
Thanks for the ping. Overall, I like the design. Here's some questions/notes. Does a Clarifying question about offsets: If I have the class definitions: class Base { virtual int foo(); virtual int bar(); };
class Derived : Base { int foo() override; int bar() override; }; Do I get a pointer to Question: Does this work with multiple inheritance? For example: class Duck { virtual ~Duck(); virtual int quack(); };
class Dog { virtual ~Dog(); virtual int woof(); };
class DuckDog : Duck, Dog { int quack() override; } How do I get the vptr for Similar question: Does this work out of the box for virtual inheritance? Or will things need to change to support that? |
That depends on what is returned by I've eliminated the Also, I should mention that this PR doesn't change the physical layout of these objects at all. I'm just changing the abstractions used to access the objects.
Good question. You would use
Virtual inheritance requires the use of VTTs, which are implemented, but may benefit from a similar change in abstractions. I haven't dug into that much yet. |
Wicked, thanks for the answers and sorry for some of them being a bit vague. I think this is a good change and fixes a lot of the headaches I have had when debugging C++ support in CIRGen. It might be good to add some C++ examples in the op descriptions to document what you described above. |
// AFTER-NEXT: %[[#ELEM_PTR:]] = cir.cast(bitcast, %[[#VPTR]] : !cir.vptr), !cir.ptr<!s64i> | ||
// AFTER-NEXT: %[[#MINUS_TWO:]] = cir.const #cir.int<-2> : !s64i | ||
// AFTER-NEXT: %[[#BASE_OFFSET_PTR:]] = cir.ptr_stride(%[[#ELEM_PTR]] : !cir.ptr<!s64i>, %[[#MINUS_TWO:]] : !s64i), !cir.ptr<!s64i> |
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.
This sequence of operations effectively get the vtable element at address point -2 which records the base-to-complete offset. I believe we will have a dedicated op for this operation (and also for other special address points such as the RTTI ptr) later so we don't have to bit-cast beween !cir.vptr
and !cir.ptr<...>
, right?
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.
Yes, that is my intent. The old sequence here did the same thing, but it was overloading the cir.vtable.address_point
operation in a way that was rejected by the verifier when I added the constraint that cir.vtable.address_point
should return a pointer to cir.vptr
. I plan to add an operations in a future change that explicitly retrieves a pointer to the base-to-complete offset entry. I'm thinking something like cir.vtable.get_complete_offset_addr
which will take a vptr
as an input and return a pointer to integer.
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.
overall this seems reasonable, I just have few technical and style comments
@@ -2624,6 +2625,101 @@ def CIR_VTableAddrPointOp : CIR_Op<"vtable.address_point",[ | |||
let hasVerifier = 1; | |||
} | |||
|
|||
//===----------------------------------------------------------------------===// | |||
// VTableGetVptr |
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.
nit: to be consistent with type name this should be VTableGetVPtr
let arguments = (ins | ||
Arg<CIR_PointerType, "the vptr address", [MemRead]>:$src); | ||
|
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.
let arguments = (ins | |
Arg<CIR_PointerType, "the vptr address", [MemRead]>:$src); | |
let arguments = (ins | |
Arg<CIR_PointerType, "the vptr address", [MemRead]>:$src | |
); | |
Arg<CIR_PointerType, "the vptr address", [MemRead]>:$src); | ||
|
||
let results = (outs CIR_PtrToVPtr:$vptr_ty); | ||
|
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.
let arguments = (ins | ||
Arg<CIR_PointerType, "the vptr address", [MemRead]>:$src); | ||
|
||
let results = (outs CIR_PtrToVPtr:$vptr_ty); |
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.
This is actual pointer result not a type, so $result
or $vptr
or $results_vptr
is more appropriate name.
let results = (outs CIR_PtrToVPtr:$vptr_ty); | |
let results = (outs CIR_PtrToVPtr:$result); |
The `vtable.get_virtual_fn_addr` operation retrieves the address of a | ||
virtual function pointer from an object's vtable (__vptr). | ||
This is an abstraction to perform the basic pointer arithmetic to get | ||
the address of the virtual function pointer, which can then be loaded and | ||
called. |
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.
Maybe reference relatoin to !cir.vptr
here.
def CIR_AnyVPtrType : CIR_TypeBase<"::cir::VPtrType", | ||
"vptr 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_AnyVPtrType : CIR_TypeBase<"::cir::VPtrType", | |
"vptr type">; | |
def CIR_AnyVPtrType : CIR_TypeBase<"::cir::VPtrType", "vptr type">; |
let builders = [ | ||
OpBuilder<(ins "mlir::Type":$type, | ||
"mlir::Value":$value, | ||
"unsigned":$index), | ||
[{ | ||
mlir::APInt fnIdx(64, index); | ||
build($_builder, $_state, type, value, fnIdx); | ||
}]> | ||
]; | ||
|
||
let extraClassDeclaration = [{ | ||
/// Return the index of the record member being accessed. | ||
uint64_t getIndex() { return getIndexAttr().getZExtValue(); } | ||
}]; |
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.
If you change type of $index
from IndexAttr
to I64Attr
in arguments these all can be deleted, as it will auto generate getIndex
returning underlying uint64_t
and is buildable from unsigned
.
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.
This is also a nice improvement. I was following the example of GetMemberOp and a few others, which can also be cleaned up in the way you suggest.
def CIR_VPtrType : CIR_Type<"VPtr", "vptr", | ||
[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_VPtrType : CIR_Type<"VPtr", "vptr", | |
[DeclareTypeInterfaceMethods<DataLayoutTypeInterface>]> { | |
def CIR_VPtrType : CIR_Type<"VPtr", "vptr", [ | |
DeclareTypeInterfaceMethods<DataLayoutTypeInterface> | |
]> { |
cir::AddressPointAttr::get(CGF.getBuilder().getContext(), 0, | ||
VTableIndex)); | ||
auto VTableSlotPtr = | ||
CGF.getBuilder().create<cir::VTableGetVirtualFnAddrOp>( |
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.
nit: add auto &builder = CGM.getBuilder();
at top of the function to clean up multiple uses of the builder
auto fnTy = cir::FuncType::get({}, intTy); | ||
|
||
auto resTy = cir::PointerType::get(cir::PointerType::get(fnTy)); | ||
auto resTy = cir::PointerType::get(cir::VPtrType::get(getContext())); | ||
|
||
if (resultType != resTy) |
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.
nit: I suggest to add let cppFunctionName = "isPtrToVPtrType";
into CIR_PtrToVPtr
constraint which can be used in places like this if (isPtrToVPtrType(resultType))
without need to explicitly construct cir::PointerType::get(cir::VPtrType::get(getContext()));
before.
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.
I don't think this is even needed here, since the return type is now encoded as CIR_PtrToVPtr in the op definition, but I'll keep this tip in mind for future uses.
This change introduces a new type, cir.vptr, and two new operations,
cir.vtable.get_vptr
andcir.vtable.get_virtual_fn_addr
to make operations involving vptrs more explicit. This also replaces cases wherecir.vtable.address_point
was being used as a general GEP-like operation and not actually returning the address point of a vtable.