[CIR][NFC] Add more examples for vtable-related ops #1782
Conversation
|
@tommymcm These are the examples, as you suggested. |
There was a problem hiding this comment.
Great documentation @andykaylor, this really clears up a lot. I have a few comments/recommendations throughout.
| @@ -2607,14 +2607,57 @@ def CIR_VTableAddrPointOp : CIR_Op<"vtable.address_point", [ | |||
| the vtable group (as specified by Itanium ABI), and `address_point.offset` | |||
| (address point index) the actual address point within that vtable. | |||
|
|
|||
| The `name` argument to this operation must be the name of a C++ vtable | |||
There was a problem hiding this comment.
I'm having a little trouble understanding this from the examples.
In the example: cir.vtable.address_point(@_ZTV1Base, ...
Is @_ZTV1Base the name of the C++ vtable? Or is it a reference to cir.global @_ZTV1Base? Or is @_ZTV1Base considered to be the 'CIR name' for the vtable (just having an @ prepended?
There was a problem hiding this comment.
It's kind of all of those. I believe it is technically an mlir::FlatSymbolRefAttr. We use it as a string to find the global. This is one of the most inefficient bits of MLIR I've come across. In LLVM IR, this would be a direct pointer to the global, but that doesn't seem to be happening in MLIR.
There was a problem hiding this comment.
That makes sense to me. It is weird that MLIR requires symbols in these cases. Thanks for the clarification.
| // Type info for S | ||
| #cir.global_view<@_ZTI1S> : !cir.ptr<!u8i>, | ||
| // Pointer to S::f1 | ||
| #cir.global_view<@_ZN1S2f1Ev> : !cir.ptr<!u8i>, |
There was a problem hiding this comment.
Maybe append a comment to the left/right of this line saying vptr --> , or on the comment line above, make a note "vptr points here".
This will visually show where the vptr points.
| address_point = <index = 0, offset = 2>) : !cir.vptr | ||
| // The vptr is at element zero. | ||
| %3 = cir.cast(bitcast, %1 : !cir.ptr<!rec_Base>), cir.ptr<!cir.vptr>> |
There was a problem hiding this comment.
Is there a reason this isn't a cir.vtable.get_vptr?
There was a problem hiding this comment.
If it's because of how this gets handled by clang, maybe we can raise it during canonicalization in a later PR?
There was a problem hiding this comment.
I'm currently working on a PR to introduce cir.vtable.get_vptr here. I have a comment in the code already saying it should be that, but I wasn't sure how that needed to interact with virtual and non-virtual base offsets. I think I have it worked out now though.
| // Constructor for Base | ||
| cir.func dso_local @_ZN4BaseC2Ev ... | ||
| ... | ||
| %2 = cir.vtable.address_point(@_ZTV1Base, |
There was a problem hiding this comment.
A comment here saying // Get the address where the vptr points, maybe?
Otherwise you have to sit with it for a bit of time to understand.
| !rec_Base2 = !cir.record<struct "Base2" {!cir.vptr} | ||
| !rec_Derived = !cir.record<struct "Derived" {!rec_Base1, !rec_Base2} | ||
| cir.func dso_local @_Z2f3P7Derived(%d: !cir.ptr<!rec_Derived>) | ||
| %2 = cir.base_class_addr %d : !cir.ptr<!rec_Derived> nonnull [8] |
There was a problem hiding this comment.
Separate note, it's weird that the offset here is [8] instead of [1]. Can these byte offsets be variable?
There was a problem hiding this comment.
We're getting the offset from clang::ASTRecordLayout::getVBaseClassOffset or clang::ASTRecordLayout::getBaseClassOffset but I think it's ultimately CXXABI-dependent, so not necessarily a simple matter of counting base classes.
There was a problem hiding this comment.
Makes sense. I assumed it was something coming from clang. I don't think this is an actual issue, was just a little odd.
|
Sorry for the rebase churn, this PR needs updating! |
This adds a few C++ examples to the documentation for the cir.vtable operations.
andykaylor
force-pushed
the
vtable-op-examples
branch
from
7512be0 to
d226bba
Compare
This adds a few C++ examples to the documentation for the cir.vtable operations.