This is an archive of the discontinued LLVM Phabricator instance.

[Clang][CodeGen] Another follow-up for `vtable`, `typeinfo` et al. are globals
ClosedPublic

Authored by AlexVlx on Jul 20 2023, 10:54 AM.

Details

Summary

Turns out that in https://reviews.llvm.org/rG8acdcf4016876d122733991561be706b64026e73 I missed another use case (dynamic_cast relies on typeinfo, which its signature assumed to be in the generic address space), sadly. This patch corrects the oversight and adds an associated test.

P.S.: the current and @bjope 's inquiry on https://reviews.llvm.org/D153092 makes me wonder if we want to consider a more centralised approach to all of the polymorphism related bits, so that future changes can be localised to one / a few spots, and then propagate around. I admit I've not thought deeply about it though and it would probably be non-trivial.

Diff Detail

Event Timeline

AlexVlx created this revision.Jul 20 2023, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 10:54 AM
Herald added a subscriber: arichardson. · View Herald Transcript
AlexVlx requested review of this revision.Jul 20 2023, 10:54 AM

Gentle ping.

__dynamic_cast is part of standard C++ library. If we ever implement it for GPU, chances are we will use libc++abi with the same signature as other targets, i.e., the 2nd and 3rd arguments are generic pointers.

I feel it is safer to do an address space cast when calling the function instead of changing its signature.

__dynamic_cast is part of standard C++ library. If we ever implement it for GPU, chances are we will use libc++abi with the same signature as other targets, i.e., the 2nd and 3rd arguments are generic pointers.

I feel it is safer to do an address space cast when calling the function instead of changing its signature.

That's a fair point, and one I admit to not having considered (I'm not sure this would harm linking in practice though, since __dynamic_cast is extern "C" and thus unmangled; it might lead to broken IR depending on how we codegen the body). The concern I have with doing the cast at the callsite is that there might be many callsites, and it's going to make things a bit noisier than they need to be (IMHO). Also, this is an internal implementation detail of the standard library, and not the interface that users are expected to touch; I wonder if given the target specific nature of these bits it wouldn't be natural for our future implementation to carry the AS in the signature?

yaxunl accepted this revision.Jul 26 2023, 12:49 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Jul 26 2023, 12:49 PM

Ping @rjmccall or @efriedma for a non AMD perspective, if possible.

This revision was landed with ongoing or failed builds.Aug 3 2023, 3:26 PM
This revision was automatically updated to reflect the committed changes.