pcc added a comment.
In D63932#1610182 <https://reviews.llvm.org/D63932#1610182>, @ostannard wrote:
> > Partial linking will indeed prevent dropping the virtual functions, but it
> > should not prevent clearing the pointer to the virtual function in the
> > vtable. The linker should then be able to drop the virtual function body as
> > part of `--gc-sections` during the final link.
>
> If partial linking isn't doing internalisation, I'd expect that to prevent a
> lot of other LTO optimisations, not just VFE. Is this a common use-case which
> we need to care about?
It isn't that common, but it seems worth doing if it can be done easily.
That said, I note that it does appear that your implementation will end up
preserving the pointer in the vtable in this case because you're relying on the
use list to make decisions about what to GC. So it doesn't seem easy to do at
this point, but if for example we made this compatible with ThinLTO at some
point we would probably not be able to rely on the use list, and the resulting
changes to this feature might make this easier to do.
>> I think I would favour something closer to your first suggestion, but
>> instead of telling GlobalDCE specifically about this, we represent the
>> "post-link" flag in the IR (e.g. as a module flag) in order to keep the IR
>> self-contained. LTO would then be taught to add this flag at the right time,
>> and the logic inside GlobalDCE would be:
>>
>> - If post-link flag not set, allow VFE if linkage <= TranslationUnit.
>> - If post-link flag set, allow VFE if linkage <= LinkageUnit.
>>
>> This would also help address a correctness issue with the CFI and WPD
>> passes, which is that it is currently unsound to run them at compile time.
>> If we let them run at compile time, we would in principle be able to do CFI
>> and WPD on internal linkage types without LTO.
>
> Ok, sounds reasonable, though I suspect WPD and CFI will need a slightly
> different definition of type visibility - they care about the possibility of
> unseen code adding new derived classes, so the visibility of base classes
> isn't important, and they might be able to make use of the final specifier.
Internal linkage types use distinct metadata (i.e. metadata that can't be
merged with other TUs), so there's no possibility of a new derived class being
added. I suspect that we could make these passes check the distinctness of
metadata if the post-link flag is not set, but that probably deserves more
thought.
================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:409
+ F->replaceAllUsesWith(
+
ConstantPointerNull::get(PointerType::getUnqual(F->getFunctionType())));
+ }
----------------
Could be just `ConstantPointerNull::get(F->getType())`.
================
Comment at: llvm/test/LTO/ARM/lto-linking-metadata.ll:2
+; RUN: opt %s -o %t1.bc
+; RUN: llvm-lto %t1.bc -o %t1.save.opt -save-merged-module -O1
--exported-symbol=foo
+; RUN: llvm-dis < %t1.save.opt.merged.bc | FileCheck %s
----------------
Could you add a test for the new LTO API as well as the legacy one?
================
Comment at:
llvm/test/Transforms/GlobalDCE/virtual-functions-derived-pointer-call.ll:30
+; pointer of type "int (B::*q)(int)", so the call could only be dispatched to
+; or B::foo. It can't be dispatched to A::bar or B::bar as the function pointer
+; does not match, and it can't be dispatched to A::foo as the object type
----------------
Remove word "or".
================
Comment at:
llvm/test/Transforms/GlobalDCE/virtual-functions-visibility-post-lto.ll:13
+@_ZTS1A = hidden constant [3 x i8] c"1A\00", align 1
+@_ZTI1A = hidden constant { i8*, i8* } { i8* bitcast (i8** getelementptr
inbounds (i8*, i8** @_ZTVN10__cxxabiv117__class_type_infoE, i64 2) to i8*), i8*
getelementptr inbounds ([3 x i8], [3 x i8]* @_ZTS1A, i32 0, i32 0) }, align 8
+
----------------
Could you simplify this test (and others) by removing the RTTI, please? We
should also have a test showing that RTTI is preserved.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63932/new/
https://reviews.llvm.org/D63932
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
