[CIR] initial support for pointer-to-data-member type

[Lancern]

This patch adds initial support for the pointer-to-data-member type. Specifically, this commit includes:

• New ops, types, and attributes:
  • Add a new type !cir.member_ptr
  • Add a new attribute #cir.data_member_ptr
  • Add a new operation cir.get_indirect_member
• CodeGen for pointer-to-data-member types and values
  • Lower C++ pointer-to-member type to !cir.member_ptr
  • Lower C++ expression &C::D to cir.const(#cir.data_member_ptr)
  • Lower C++ expression c.*p and c->*p to cir.get_indirect_member

This patch only includes an initial support. The following stuff related to pointer-to-member types are not supported yet:

• Pointer to member function;
• Conversion from T Base::* to T Derived::*;
• LLVMIR lowering.

Hopefully they will eventually be supported in later PRs :)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[sitio-couto]

@Lancern awsome! I left a few suggestions to improve testing and the codegen side.

[sitio-couto]

@Lancern thanks for all the changes! Just one more round of reviews.

[bcardosolopes]

Thanks for adding support for pointers to data members! This is going to allows to build even more C++, way to go.

I believe we can use currently existing types and ops to support these, and no need to introduce new ops/attr/types. If you have a specific use case you care about, can you please provide examples of how using this extra layer information helps?

• Add a new type !cir.member_ptr

All ops that use/return these might as well return a cir.ptr to the actual final type.

• Add a new attribute #cir.data_member_ptr

The AST allows you to query the type of the struct element given you already have the index, so it can just be used directly.

• Add a new operation cir.get_indirect_member

Without a !cir.member_ptr, this one becomes unnecessary.

[Lancern]

@bcardosolopes Thanks for your comment! But TBH I'm sorry I don't get your point exactly.

All ops that use/return these might as well return a cir.ptr to the actual final type.

I don't clearly see how this is possible. A member pointer is totally different from an ordinary pointer. If we insist the reuse of the existing types instead of the new types, one possible way is to use the types compatible with the underlying ABI layout to represent a ptr-to-data-member. On most platforms, a ptr-to-data-member is simply represented by an integer which represents the offset of the target field from the beginning of the containing object. So we might use a !s32i or a !s64i to represent a ptr-to-data-member. But this would make a ptr-to-data-member indistinguishable from a normal integer in the CIR, which might affect analysis that is interested on this. Also, this would make the CIR of obj.*member and obj->*member complex because you need to do a series of raw pointer arithmetic and casts.

Another possible way to represent the ptr-to-member type in CIR is to invent an imagined cir.member type and then cir.ptr<cir.member> represents a ptr-to-member. Are you suggesting this method? This method diverges from the C++ language since the language does not have a corresponding concept.

[Lancern]

Seems that I missed the null pointer value support for ptr-to-data-member. I'll check this and update the PR.

[sitio-couto]

@bcardosolopes take a look at this godbolt example. I believe it corroborates @Lancern's arguments.

might as well return a cir.ptr to the actual final type.

This might work, but we would be losing source code information and diverging from the original codegen. These two types are inherently different, as seen in the example above. Having separate types should facilitate both analysis and lowering. It might also come in handy when we have a proper solution for dealing with target/ABI-specific stuff.

The AST allows you to query the type of the struct element given you already have the index, so it can just be used directly.

I didn't quite understand this point. @bcardosolopes could you elaborate?

Without a !cir.member_ptr, this one becomes unnecessary.

I believe this is necessary regardless of the !cir.member_ptr. My understanding is that the get_indirect_member uses variable indexes, while get_member only allows constant indexes. This is essential for pointer-to-member types as they might be unknown at compile time (e.g. when passed as function arguments as shown in the example). We could merge these two operations into a more generic get_member that can do both, but the separation might be better as an initial step.

[Lancern]

Seems that I missed the null pointer value support for ptr-to-data-member. I'll check this and update the PR.

Just added support for null ptr-to-data-member values.

[Lancern]

Rebased onto the latest main.

[sitio-couto]

@Lancern a few more suggestions and some minor nitpicks.

[sitio-couto]

Expand this explaining that the literal represents the index of the data member.

[Lancern]

Updated.

[bcardosolopes]

Addressing @Lancern comments:

Thanks for your comment! But TBH I'm sorry I don't get your point exactly.

No problem, let me clarify!

I don't clearly see how this is possible. A member pointer is totally different from an ordinary pointer. If we insist the reuse of the existing types instead of the new types, one possible way is to use the types compatible with the underlying ABI layout to represent a ptr-to-data-member.

I think my point can be easier to understand if I ask you a different question: how do you plan to lower this to LLVM? CIR already has most of the things you need to have a lower level representation to handle this just like LLVM does.

On most platforms, a ptr-to-data-member is simply represented by an integer which represents the offset of the target field from the beginning of the containing object. So we might use a !s32i or a !s64i to represent a ptr-to-data-member. But this would make a ptr-to-data-member indistinguishable from a normal integer in the CIR, which might affect analysis that is interested on this.

Can you give me an example of such analysis and a specific use case you care about? Note that I'm not against adding nice abstractions, I'm all for it, but I rather not introduce complexity for things we don't have concrete use cases in mind. Note that if we add the abstractions like you are proposing, you'd also need to lower it down to more lower level CIR as part of the LoweringPrepare pass before we go down to LLVM. Without some roadmap on how you'd like to use this abstractions, I rather they only get added upon compromise on some upcoming feature that's going to use it.

Another possible way to represent the ptr-to-member type in CIR is to invent an imagined cir.member type and then cir.ptr<cir.member> represents a ptr-to-member. Are you suggesting this method? This method diverges from the C++ language since the language does not have a corresponding concept.

I'm looking at https://godbolt.org/z/hjaMcqKov and CIR has all the operations necessary to the same approach, can you elaborate on the specific limitations you've found?

[bcardosolopes]

Addressing @sitio-couto

This might work, but we would be losing source code information and diverging from the original codegen. These two types are inherently different, as seen in the example above. Having separate types should facilitate both analysis and lowering.

I'm willing to bite that compromise unless there's a concrete use case in mind. So far, lowering to LLVM sounds like the only one.

It might also come in handy when we have a proper solution for dealing with target/ABI-specific stuff.

Lowering these ABI details early sounds fine to me, how much these featues get used in practice to justify the abstraction? I don't have numbers nor am I asking for them, but I don't stumble into data-member pointers that often, I might be biased tho.

I believe this is necessary regardless of the !cir.member_ptr. My understanding is that the get_indirect_member uses variable indexes, while get_member only allows constant indexes. This is essential for pointer-to-member types as they might be unknown at compile time (e.g. when passed as function arguments as shown in the example). We could merge these two operations into a more generic get_member that can do both, but the separation might be better as an initial step.

It's a good way to fix it while keeping nice abstraction, but I'm not yet convinced there are users to take advantage of the benefits.

[sitio-couto]

@bcardosolopes

lowering to LLVM sounds like the only one.

IMO, this and being semantically closer to C/C++ is enough to at least justify the type and the attribute. After all, the complexity of these two components is minimal.

how much these featues get used in practice to justify the abstraction?

I do not know, but this may also be a reason for us to be conservative by adding said abstraction.

I'm not yet convinced there are users

I don't think there are. GEP's with runtime indexes are not uncommon in the original codegen. In CIR, however, most are replaced with pointer strides (example). We could go the same route with data-member pointers.

[Lancern]

@bcardosolopes

Can you give me an example of such analysis and a specific use case you care about?

Unfortunately I cannot give a concrete example at this moment that shows the immediate usefulness of the newly added stuff in this PR. But I believe that it brings benefits similar to the cir.get_member operation. Note that the functionality of cir.get_member can be fully achieved by other existing CIR operations, but it is still invented and used.

Without some roadmap on how you'd like to use this abstractions, I rather they only get added upon compromise on some upcoming feature that's going to use it.

This makes sense to me. So if it finally comes to a consenus that this PR is too early to be landed at this stage of this project, I'll close this PR and draft a new PR that directly implements ptr-to-data-member upon existing CIR types and operations.

[bcardosolopes]

This makes sense to me. So if it finally comes to a consenus that this PR is too early to be landed at this stage of this project, I'll close this PR and draft a new PR that directly implements ptr-to-data-member upon existing CIR types and operations.

I'm still open to discuss if you feel very strong about this. For instance, you initially suggest to add !cir.member_ptr,
#cir.data_member_ptr and cir.get_indirect_member, but I didn't see any argument into the direction of a more minimal approach that still keeps some of the semantics you care. It doesn't mean this should be all in or all out. Alternatively, you could do the directly implementation (because that unlocks the fast path to LLVM) and do later incremental work on the pieces you want to raise.

[Lancern]

Rebased onto the latest main.

[bcardosolopes]

Apologies for the delay here. I finally had some time to take a closer look at this. Before you make changes to this patch, let's perhaps wrap this discussion and converge on a resolution first, so you don't waste resources. My comments/suggestions follow:

case 1

int Point::*pt_member = &Point::x;
// cir.global external @pt_member = #cir.data_member_ptr<!ty_22Point22, 0> : !cir.member_ptr<!s32i in !ty_22Point22>

If the type of !cir.member_ptr is !s32i in !ty_22Point22, it's not really providing much information, given the type doesn't encode any specific information about the element index (for instance, !ty_22Point22 has 3 ints inside it, which one is this referring to?). Not saying you should encode the information here, just making a point about the fact that it's not really providing much more.

My suggestion:

1. Change #cir.data_member_ptr<!ty_22Point22, 0> to #cir.data_member_ptr<!ty_22Point22[0]>.
2. Result decay to the actual member pointer type. E.g. !cir.member_ptr<!s32i in !ty_22Point22> -> !cir.ptr<!s32i>

int Point::*pt_member = &Point::x;
// cir.global external @pt_member = #cir.data_member_ptr<!ty_22Point22[0]> : !cir.ptr<!s32i>

case 2

auto test1() -> int Point::* {
  return &Point::y;
}
// cir.func @_Z5test1v() -> !cir.member_ptr<!s32i in !ty_22Point22>
//   %{{.+}} = cir.const(#cir.data_member_ptr<!ty_22Point22, 1> : !cir.member_ptr<!s32i in !ty_22Point22>) : !cir.member_ptr<!s32i in !ty_22Point22>
// }

My suggestion:

auto test1() -> int Point::* {
  return &Point::y;
}
// cir.func @_Z5test1v() -> !cir.ptr<!s32i>
//   %{{.+}} = cir.const(#cir.data_member_ptr<!ty_22Point22[1]> : !cir.ptr<!s32i>) : !cir.ptr<!s32i>
// }

case 3

int test2(const Point &pt, int Point::*member) {
  return pt.*member;
}
// cir.func @_Z5test2RK5PointMS_i
//   %{{.+}} = cir.get_indirect_member %{{.+}}[%{{.+}} : !cir.member_ptr<!s32i in !ty_22Point22>] : !cir.ptr<!ty_22Point22> -> !cir.ptr<!s32i>
// }

Ok, I think we can still use this one, with some changes:

My suggestion:

1. Rename to cir.get_runtime_member.
2. Create an interface MemberOpInterface to be shared between cir.get_member and cir.get_runtime_member. Both ops should have getAddrTy() and getResultTy() and a new one isDynamic() (or isStatic()?). The idea here is that passes that operate on cir.get_member should use the interface instead (and isDynamic() is available to distinguish properties).
3. Alternatively, get_member could also be changed to support a dynmamic case, but maybe start with an interface is better (@sitio-couto, wdyt?)

// cir.func @_Z5test2RK5PointMS_i(%arg0..., %arg1 : !u64i)
//   = cir.get_runtime_member %0[%val : u64i] : (!cir.ptr<!ty_22Point22>) -> !cir.ptr<!s32i>
// }

Seems like the same applies to the incomplete types as well.

On the !u64i choice, we could use datalayout queries to decide between a !u32i or !u64i. I don't know offhand if CGF.SizeTy is what is used for LLVM, but it's either that or some similar ABI based cached type (LLVM codegen should have that information for you to use as inspiration).

In a gist, the changes would be:

• Get rid of !cir.member_ptr.
• Changes to cir.get_indirect_member and #cir.data_member_ptr

Does this makes to you? Anything I'm missing?

[Lancern]

@bcardosolopes Thanks for your comment! Overall the suggestions make sense, but one problem in case 1 and case 2:

Result decay to the actual member pointer type. E.g. !cir.member_ptr<!s32i in !ty_22Point22> -> !cir.ptr<!s32i>

This decay does not make sense because member pointers are not pointers as represented by !cir.ptr. Data member pointers are just offsets. In case 3 you lower the member parameter to a !u64i and that makes more sense. So a more reasonable decay would be !cir.member_ptr<!s32i in !ty_22Point22> -> !u64i. (p.s. as you said we could use datalayout queries to decide between a !u32i or !u64i).

For case 1 a more reasonable lowering:

int Point::*pt_member = &Point::x;
// cir.global external @pt_member = #cir.data_member_ptr<!ty_22Point22[0]> : !u64i

For case 2:

auto test1() -> int Point::* {
  return &Point::y;
}
// cir.func @_Z5test1v() -> !u64i
//   %{{.+}} = cir.const(#cir.data_member_ptr<!ty_22Point22[1]> : !u64i) : !u64i
// }

Case 3 makes sense to me.

[bcardosolopes]

In case 3 you lower the member parameter to a !u64i and that makes more sense. So a more reasonable decay would be !cir.member_ptr<!s32i in !ty_22Point22> -> !u64i

Good catch, sounds great to me!

[sitio-couto]

@bcardosolopes

Change #cir.data_member_ptr<!ty_22Point22, 0> to #cir.data_member_ptr<!ty_22Point22[0]>

This representation will complicate printing/parsing and type-checking. I suggested #cir.data_member_ptr<0> : #cir.data_member_ptr<!s32i in !ty_22Point22> to reuse the type printer/parser: 0 is the field index, !s32i the field type, and !ty_22Point22 the class where to look for the given index.

Result decay to the actual member pointer type

In case 3 you lower the member parameter to a !u64i and that makes more sense.

Is the intent to drop the data-member-pointers type info right after the declaration? If this is the case, the new type will be useless, I think, since it will only be used to declare such data and be dropped right after. Other codegen sections for data-member-pointer will likely also require change, such as assignments.

Both comments above apply to Case 2 as well.

but maybe start with an interface is better

An interface seems excessive for a temporary solution. Are there any current places where said MemberOpInterface would be used? If not, regular llvm::isa<Ty> should suffice where it is needed, otherwise, seems like generalizing 'cir.get_member' would be more productive. LLVM Dialect's GEP op can serve as an example.

[bcardosolopes]

This representation will complicate printing/parsing and type-checking. I suggested #cir.data_member_ptr<0> : #cir.data_member_ptr<!s32i in !ty_22Point22> to reuse the type printer/parser: 0 is the field index, !s32i the field type, and !ty_22Point22 the class where to look for the given index.

Works for me.

Is the intent to drop the data-member-pointers type info right after the declaration? If this is the case, the new type will be useless, I think, since it will only be used to declare such data and be dropped right after. Other codegen sections for data-member-pointer will likely also require change, such as assignments.

One IPO opportunity I can see in future is tracking a perhaps offset on a callsite param and specialize the callee to transform a get_runtime_member into a regular get_member. This is why I believe it's important to keep the new constant data attribute to represent the offset - however, thinking more about it now, we don't even need that, all we really need is to look into get_runtime_member, which already has the struct type, so if we can prove that the dynamic size is in fact a constant, such optimization would still be possible. So you are right, we don't even need the attribute. One thing we need though: make sure that the offset doesn't consider the underlying type size, and only tracks the member position instead.

An interface seems excessive for a temporary solution. Are there any current places where said MemberOpInterface would be used? If not, regular llvm::isa<Ty> should suffice where it is needed, otherwise, seems like generalizing 'cir.get_member' would be more productive. LLVM Dialect's GEP op can serve as an example.

I'm in favor of generalizing cir.get_member, I just wanted to make sure you didn't have extra concerns.

[sitio-couto]

@bcardosolopes

So you are right, we don't even need the attribute.

My point is not whether we need the new attribute and type. Since we are introducing them, it seems contradictory that the first action we take is to discard this information by decaying it to an integer, especially if it will complicate other areas such as codegen. Considering this, what is the benefit of this decay? The IPO mentioned seems viable regardless.

I'm in favor of generalizing cir.get_member, I just wanted to make sure you didn't have extra concerns.

I would prefer to leave them separate. get_runtime_member's only users are data-member-pointer accesses. Generalizing get_member will considerably complicate it to add functionality required by a single client that might be deprecated in the future. However, between the proposed interface and generalizing get_member, I do prefer the latter.

BTW, I mentioned before that runtime GEP operations are represented as low-level pointer strides and loads in CIR. If this is by design, perhaps we should do the same thing with data-member-pointers.

[bcardosolopes]

Ok, almost converging :)

My point is not whether we need the new attribute and type. Since we are introducing them, it seems contradictory that the first action we take is to discard this information by decaying it to an integer

It seems to me that we can easily retrieve information (versus tree matching a bunch of conditions) by having a data attribute (there's no other way to initialze those otherwise) and a dynamic get_member. The specific type isn't useless and could have some nice effects (like skipping function arguments during static analysis), but I'm failing to see a real value.

especially if it will complicate other areas such as codegen.

Can you elaborate on how it will complicate codegen?

@sitio-couto @Lancern: if we had such a type, how do you plan to later lower it to an actual offset in LowerPrepare? Will we need AST information or does CIR already has everything? I'm asking because the current proposed type seems to be: "there's a cir.int in the relevant struct", but it doesn't seem to contain any offset info into that struct (which could have many cir.ints inside).

Considering this, what is the benefit of this decay? The IPO mentioned seems viable regardless.

I have the reverse question, what's the benefit of keeping it? The only ones I see are mentioned above: defer ABI decision when it's a function argument.

@sitio-couto: if you feel strong about this, I'm willing to take it, I probably just need some comments on the questions above, so I know what's the plan of record.

I would prefer to leave them separate. get_runtime_member's only users are data-member-pointer accesses. Generalizing get_member will considerably complicate it to add functionality required by a single client that might be deprecated in the future. However, between the proposed interface and generalizing get_member, I do prefer the latter.

I'm fine with having them separate, I don't have a strong take here, tradeoffs seem fair. My idea with the interface was considering that the thing was actually a pointer type, and if that was the case, it'd be silly to have more than one type that behaves like a pointer without an interface to unify queries. However, since the thing represents an offset, it doesn't make sense to use an interface - I forgot to relay that info in previous comments.

BTW, I mentioned before that runtime GEP operations are represented as low-level pointer strides and loads in CIR. If this is by design, perhaps we should do the same thing with data-member-pointers.

I don't like pointer strides here because it loses the "this is dynamically walking a struct" semantic, and makes it semantically easy to transform into static in the future. Also, LowerPrepare could lower get_runtime_member to a stride, not a problem there.

On another note, we haven't been using strides for struct related code, at least not from what I can currently see in tests. If we want to unify that somehow, should probably be its own PR proposal.

[sitio-couto]

@Lancern,

@bcardosolopes and I talked a bit offline and agreed on the following:

• Represent data-member pointers as a new ABI-independent data_member type/attribute.
• Represent data-member pointers runtime access with a new isolated get_runtime_member operation.

This is mostly what you're already doing, however, there are a couple of changes to be made:

• Rename data_member_ptr to just data_member.
  This is so we make it clear that data_member is not a pointer, but an offset.
• Add a type constraint to get_runtime_member enforcing that its indexes must be of type data_member.
  This is so we prevent any misuse of this new operation. We may generalize it later if necessary.

@Lancern let us know when you've addressed the remaining reviews, and if you have any other questions!

[orbiri]

Hey, I've stumbled upon this test when refactoring the cir.const assembly.
Can you explain what you were trying to test here? The reason I'm asking is that what you have here is an attribute alias, not a global value. That's the reason that the output MLIR is "inlining" it into:

// CHECK-NEXT:   cir.func @get_global_member(%arg0: !cir.ptr<!ty_22Foo22>) {
// CHECK-NEXT:     %0 = cir.const(#cir.data_member<0> : !cir.data_member<!s32i in !ty_22Foo22>) : !cir.data_member<!s32i in !ty_22Foo22>
// CHECK-NEXT:     %1 = cir.get_runtime_member %arg0[%0 : !cir.data_member<!s32i in !ty_22Foo22>] : !cir.ptr<!ty_22Foo22> -> !cir.ptr<!s32i>
// CHECK-NEXT:     cir.return

[bcardosolopes]

@Lancern ^

[Lancern]

@orbiri Well this should just be an effort to save some typings when writing this test. But clearly it does little help, so you can change it if it's convenient for you.

[orbiri]

@orbiri Well this should just be an effort to save some typings when writing this test. But clearly it does little help, so you can change it if it's convenient for you.

From the test name it seems like you tried to test the incorporation of some global property here, but if not - then get_global_member is 1:1 equivalent to get_runtime_member, and I'd suggest to remove it.

If indeed you intended to test some global property, I'd suggest to revisit this test to make sure that it won't break over time 🙏

[Lancern]

From the test name it seems like you tried to test the incorporation of some global property here, but if not - then get_global_member is 1:1 equivalent to get_runtime_member, and I'd suggest to remove it.

I checked this again and I'm sure they're 1:1 equivalent as you said. I'll remove it in an NFC PR if you request so.

[Lancern]

I'll remove it in an NFC PR if you request so.

See #582 .