[CIR][LLVMLowering] Add LLVM lowering for data member pointers #612
Conversation
Lancern
force-pushed
the
data-member-ptr-lowering
branch
from
802606e
to
f81fd78
Compare
|
|
||
| uint64_t memberOffset; | ||
| if (attr.isNullPtr()) | ||
| memberOffset = -1ull; |
There was a problem hiding this comment.
This constant value for null pointers is actually ABI-specific. Should we move this to some ABI-specific part?
There was a problem hiding this comment.
Should we elevate lowerDataMemberAttr as a whole to LoweringPrepareCXXABI, so that the codes are centralized?
There was a problem hiding this comment.
Should we elevate lowerDataMemberAttr as a whole to LoweringPrepareCXXABI, so that the codes are centralized?
Transforming data member pointers to member offsets during lowering prepare sounds reasonable at first, but the tricky part here is you also have to change every occurrences of !cir.data_member type to !cir.int type, which can be hard to implement during lowering prepare.
There was a problem hiding this comment.
Work that @sitio-couto is doing in #643 should provide us a way to ask those queries at LLVM lowering stage as well, since the information should be available at any time post CIRGen. I think it's fine to leave this as is and add a missing feature so that @sitio-couto can patch this when he lands his work. @sitio-couto any extra thoughts?
There was a problem hiding this comment.
@bcardosolopes sounds good to me. But the "tricky part" that @Lancern pointed out will still be an issue on the ABI lowering stage. AFAIK, there's no way for passes to iterate on specific attrs. We would either need to walk through every op or have a well-defined set of ops that may use the DataMemberAttr.
There was a problem hiding this comment.
This seems to be the same thing from https://github.com/llvm/clangir/pull/652/files#r1625515020. To be more clear, I'm not suggesting we do it in LoweringPrepare, what I'm suggesting is that since @sitio-couto ABI library is independent of CIRGen, it should also be usable from LowerToLLVM.cpp. A missing feature check here works for now (perhaps you wait for @seven-mile to land it and use it here?)
|
|
||
| uint64_t memberOffset; | ||
| if (attr.isNullPtr()) | ||
| memberOffset = -1ull; |
There was a problem hiding this comment.
Should we elevate lowerDataMemberAttr as a whole to LoweringPrepareCXXABI, so that the codes are centralized?
This patch adds LLVM lowering support for data member pointers. It also refactors some CIRGen of the data member pointers to follow the upstream CodeGen skeleton.
Lancern
force-pushed
the
data-member-ptr-lowering
branch
from
f81fd78
to
9d69882
Compare
This PR adds LLVM lowering support for data member pointers. It includes the following changes:
The#cir.data_memberattribute now has a new parameter namedmemberOffset. When the data member pointer is not null, this parameter gives the offset of the pointed-to member within its containing object. This offset is calculated by target ABI.A new attribute#cir.data_member_ptr_layoutis added. It contains ABI-specific layout information about a data member pointer that is required to lower it to LLVM IR. This attribute is attached to the module op, and it is queried during LLVMIR lowering to obtain the lowering information in it.