[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

sylvestre.ledru added a comment. @ostannard Maybe you could add that to the release notes? Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63932/new/ https://reviews.llvm.org/D63932 ___

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

This revision was automatically updated to reflect the committed changes. Closed by commit rG9f6a873268e1: Dead Virtual Function Elimination (authored by ostannard). Changed prior to commit: https://reviews.llvm.org/D63932?vs=218363=224568#toc Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

pcc accepted this revision. pcc added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63932/new/ https://reviews.llvm.org/D63932 ___

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. Ping 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. Ping 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

mehdi_amini added a comment. In D63932#1679910 , @ostannard wrote: > > This makes the IR self-contained, which is good, but it also make the > > interpretation of the IR modal, which isn't great. It means that suddenly > > the rules of interpretation of

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. > This makes the IR self-contained, which is good, but it also make the > interpretation of the IR modal, which isn't great. It means that suddenly the > rules of interpretation of what is valid to do or not changes according to > this module flag. I think the

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

mehdi_amini added a comment. > we represent the "post-link" flag in the IR (e.g. as a module flag) in order > to keep the IR self-contained. This makes the IR self-contained, which is good, but it also make the interpretation of the IR modal, which isn't great. It means that suddenly the

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. Ping 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. Ping 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard marked 4 inline comments as done. ostannard added a comment. > 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard updated this revision to Diff 218363. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63932/new/ https://reviews.llvm.org/D63932 Files: clang/include/clang/Basic/CodeGenOptions.def clang/include/clang/Driver/Options.td

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

pcc added a comment. In 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard updated this revision to Diff 212833. ostannard added a comment. - Add LTOPostLink metadata, instead of internalising vcall visibility at LTO time Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63932/new/ https://reviews.llvm.org/D63932

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. > 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

pcc added a comment. In D63932#1608235 , @ostannard wrote: > > - Take the example from my earlier message, give the "main executable" and > > "DSO" hidden visibility, build the "main executable" with LTO and the "DSO" > > without LTO, and statically

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. > - Take the example from my earlier message, give the "main executable" and > "DSO" hidden visibility, build the "main executable" with LTO and the "DSO" > without LTO, and statically link them both into the same executable. We run > into the same problem where the

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

pcc added a comment. In D63932#1606248 , @ostannard wrote: > In that example, with everything having default ELF visibility, all of the > vtables will get vcall_visibility public, which can't be optimised by VFE, > and won't ever be relaxed to one of

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard updated this revision to Diff 212370. ostannard marked 2 inline comments as done. ostannard added a comment. - Rebase - Don't emit llvm.assume when not necessary (we already weren't checking for it's presence in GlobalDCE) - s/"public"/"default"/ in IR docs Repository: rG LLVM

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. In that example, with everything having default ELF visibility, all of the vtables will get vcall_visibility public, which can't be optimised by VFE, and won't ever be relaxed to one of the tighter visibilities. The cases which can be optimised are (assuming

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

pcc added a comment. It doesn't seem sound to create the distinction between translation unit and linkage unit visibility and then allow passes to relax visibility. For example, imagine that you have an executable with (compiled with `-fvisibility=default`): struct PluginInterface {

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. Ping. 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

pcc added a comment. Hi Oliver, thanks for working on this. This is something that I've always wanted to do with the type metadata but never had time to work on. I'll try to take a more in depth look later this week. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. Ping. 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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard added a comment. Without switching to `llvm.type.checked.load`, the optimisation would trigger in some cases where it shouldn't trigger, thus miscompiling the code. This is because it needs to know about _every_ virtual call site, if we lose the information about some of them then it

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

lebedev.ri added a comment. To make this transformation safe, I have changed clang's code-generation to always load virtual function pointers using the llvm.type.checked.load intrinsic, instead of regular load instructions. I originally tried writing this using clang's existing

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

ostannard created this revision. ostannard added reviewers: mehdi_amini, pcc, majnemer, tejohnson. Herald added subscribers: dang, dexonsmith, steven_wu, hiraditya, kristof.beyls, Prazek, javed.absar. Herald added projects: clang, LLVM. Currently, it is hard for the compiler to remove unused C++