This is an archive of the discontinued LLVM Phabricator instance.

Borrow visibility from __fundamental_type_info for generated fundamental type infos
ClosedPublic

Authored by thomasanderson on Jul 9 2018, 5:07 PM.

Diff Detail

Repository
rC Clang

Event Timeline

thomasanderson edited the summary of this revision. (Show Details)Jul 9 2018, 5:10 PM

Peter does this look reasonable?

pcc added inline comments.Jul 18 2018, 1:48 PM
lib/CodeGen/ItaniumCXXABI.cpp
1620–1628

I think this code can be replaced with options.Visibility = CodeGenModule::GetLLVMVisibility(RD->getVisibility());

3180

Would it be worth splitting the BuildTypeInfo function in two at this point? I'm thinking that the first function would decide whether to create an RTTI entry. If so it would determine the linkage, visibility and dllexport and pass them to a second function, otherwise it would create an external reference. The second function would take a type, linkage, visibility and dllexport and create the RTTI unconditionally.

thomasanderson marked 2 inline comments as done.
pcc added inline comments.Jul 18 2018, 6:51 PM
lib/CodeGen/ItaniumCXXABI.cpp
3180

Sorry, I didn't complete my thought. I was imagining that we would change EmitFundamentalRTTIDescriptor to call the second function directly and drop the Force and Options argument from this function.

thomasanderson marked an inline comment as done.Jul 19 2018, 11:43 AM
pcc added inline comments.
lib/CodeGen/ItaniumCXXABI.cpp
3201–3202

IsStdLib will always be false at this point because of the if statement on line 3211 so I think you can change this to just llvm::GlobalVariable::LinkageTypes Linkage = getTypeInfoLinkage(CGM, Ty); and fold the call to IsStandardLibraryRTTIDescriptor(Ty) into the if statement.

3242–3244

Similarly, ShouldUseExternalRTTIDescriptor(CGM, Ty) should always return false here because of the if statement, so this code is dead.

3386–3387

You can remove the if statement, setting the storage class to default has no effect here because it is initialized to default.

3388–3394

This code is also dead.

3663

You might consider inlining EmitFundamentalRTTIDescriptor into this function and having it take a CXXRecordDecl. Then you wouldn't need to pass the TypeInfoOptions structure around, you can just compute the properties in this function outside of the loop.

thomasanderson marked 5 inline comments as done.
pcc accepted this revision.Jul 19 2018, 6:22 PM

LGTM

This revision is now accepted and ready to land.Jul 19 2018, 6:22 PM
pcc requested changes to this revision.Jul 19 2018, 6:23 PM

Sorry, I noticed that this patch is missing a test case. Can you add one please?

This revision now requires changes to proceed.Jul 19 2018, 6:23 PM

Added test. Also verified that all tests in llvm/tools/clang/test pass.

pcc accepted this revision.Jul 23 2018, 5:41 PM

LGTM

This revision is now accepted and ready to land.Jul 23 2018, 5:41 PM
This revision was automatically updated to reflect the committed changes.