Page MenuHomePhabricator

[WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes
ClosedPublic

Authored by aeubanks on Jun 30 2022, 3:19 PM.

Details

Summary

Turning on opaque pointers has uncovered an issue with WPD where we currently pattern match away assume(type.test) in WPD so that a later LTT doesn't resolve the type test to undef and introduce an assume(false). The pattern matching can fail in cases where we transform two assume(type.test)s into assume(phi(type.test.1, type.test.2)).

Currently we create assume(type.test) for all virtual calls that might be devirtualized. This is to support -Wl,--lto-whole-program-visibility.

To prevent this, all virtual calls that may not be in the same LTO module instead use a new llvm.public.type.test intrinsic in place of the llvm.type.test. Then when we know if -Wl,--lto-whole-program-visibility is passed or not, we can either replace all llvm.public.type.test with llvm.type.test, or replace all llvm.public.type.test with true. This prevents WPD from trying to pattern match away assume(type.test) for public virtual calls when failing the pattern matching will result in miscompiles.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 3:19 PM
aeubanks requested review of this revision.Jun 30 2022, 3:19 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 30 2022, 3:19 PM
aeubanks edited the summary of this revision. (Show Details)Tue, Jul 12, 4:09 PM
aeubanks retitled this revision from [WPD] Use new llvm.public.type.test intrinsic for publicly visible classes to [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes.Tue, Jul 12, 4:09 PM
aeubanks updated this revision to Diff 444706.Thu, Jul 14, 10:09 AM

make sure not to regress windows by checking HasHiddenLTOVisibility() to decide whether to emit public.type.test vs type.test

I did verify that -fvisibility=hidden still devirtualizes, and same with -Wl,--lto-whole-program-visibility, but not if both are missing

llvm/lib/LTO/LTOBackend.cpp
597

I'm not sure where the best place to put these is

Thanks for implementing this!

In D71913, I also modified this code, which might need something similar: https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/clang/lib/CodeGen/ItaniumCXXABI.cpp#L707-L713

clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
22

Why remove this one here and below?

clang/test/CodeGenCXX/type-metadata.cpp
150

It's a little confusing to distinguish the results by prefixing them as "CFI" or "VTABLE". Afaict the behavior is determined by whether -fvisibility=hidden has been passed in all cases for Itanium.

And MS is always hidden from what I see here? So in that case no need to split the expected results into two copies.

lld/test/ELF/lto/update_public_type_test.ll
2

Add comments about what is being tested. Also good to test via llvm-lto2 which has a similar -whole-program-visibility option, rather than just via lld here. See for example llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll.

llvm/lib/LTO/LTOBackend.cpp
597

I don't think that this will work for distributed ThinLTO, where the ThinLTO Backends are run via clang, which isn't going to have this config variable set (since it is set only during LTO linking). I think something may need to be recorded in the index as a flag there at thin link / indexing time that can be checked here.

It would then be nice to consolidate this handling into WPD itself, e.g. at the start of DevirtModule::run, but unfortunately we won't have a summary index for pure regular LTO WPD so I don't think that would work. So in here is ok but I would do it early to handle some of the early return cases.

(Please add a distributed ThinLTO test too)

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1807

Shouldn't we have converted all of these by now?

aeubanks updated this revision to Diff 445978.Tue, Jul 19, 4:13 PM
aeubanks marked an inline comment as done.

update

aeubanks added inline comments.Tue, Jul 19, 4:13 PM
clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
22

we end up RAUWing the public.type.test with true and end up with assume(true) which is harmless and gets removed by something like instcombine

clang/test/CodeGenCXX/type-metadata.cpp
150

not sure why I had CFI and WPD as acting differently in my mind, fixed

lld/test/ELF/lto/update_public_type_test.ll
2

added comments

I extended llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll, is that ok?

llvm/lib/LTO/LTOBackend.cpp
597

Added a distributed ThinLTO test: clang/test/CodeGenCXX/thinlto_public_type_test_distributed.ll.

I added ModuleSummaryIndex::WithWholeProgramVisibility but I'm not sure where I'd set it, and that's causing the test to fail.

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1807

Hmm, I vaguely remember adding this because some clang test failed (something to do with the extra LTT we add in BackendUtil.cpp), but reverting this doesn't cause any failures.

aeubanks updated this revision to Diff 446179.Wed, Jul 20, 9:32 AM

update

llvm/lib/LTO/LTOBackend.cpp
597

ah, I needed a separate WPV flag in llvm-lto2 to set llvm::lto::Config, now everything passes

tejohnson added inline comments.Wed, Jul 20, 10:11 AM
clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
22

Sure, but it should still be gone and can confirm that here? If it isn't useful to check for, then the comment should probably be changed too since it mentions assumes.

clang/test/CodeGenCXX/thinlto_public_type_test_distributed.ll
24

Is there a reason why the hidden version checks after promote but the public version above checks after importing?

clang/test/CodeGenCXX/type-metadata.cpp
212

Is the CFI-VT-ITANIUM and CFI-VT-MS split needed? I don't see any differences in their checking. Can we go back to just a single CFI-VT here?

lld/test/ELF/lto/update_public_type_test.ll
2

Yeah sure that's fine. But maybe also add a direct check there in both cases after promote that the public.type.test/assume were transformed as expected as you do in this test.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
169

Remove commented debugging code here and below

llvm/lib/LTO/LTO.cpp
1106

Probably need to do for the legacy LTO API as well, which also calls updateVCallVisibilityInModule (LTOCodeGenerator.cpp). This is used both some other linkers such as ld64. Otherwise I think the public.type.test intrinsics will never get converted for those LTO users, leading to errors probably? You can test the legacy LTO via llvm-lto (e.g. see tests in llvm/test/LTO/X86).

llvm/lib/LTO/LTOBackend.cpp
565

Can we just check the combined index flag now?

597

I see from your most recent update that you added code to set/check this new index flag. But I think you would want to set it around the same place where we update the vcall visibility during the thin link (see calls to updateVCallVisibilityInIndex). And I would do it via a new method in the WPD source file that uses the existing hasWholeProgramVisibility() that checks the OR of the config flag or the internal option. Then you don't need to add a new flag to llvm-lto2 for this purpose.

Also, as I noted for the regular LTO handling elsewhere, I think you need to add similar handling for the legacy LTO implementation. See other callers to updateVCallVisibilityInIndex.

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1807

Related to some of my other comments about ensuring we do the conversion for the legacy LTO interfaces, perhaps this we should assert here if there are any remaining public.type.test intrinsics? Not sure what the failure mode is if any of those stick around otherwise.

aeubanks updated this revision to Diff 446269.Wed, Jul 20, 2:38 PM
aeubanks marked 3 inline comments as done.

update

the assert that there are no public.type.tests in LTT fails on CodeGenCXX/thinlto-distributed-type-metadata.cpp. for some reason clang doesn't go through the LTO API at [1], it just ends up calling the normal optimization pipeline. any ideas why?

[1] https://github.com/llvm/llvm-project/blob/0c1b32717bcffcf8edf95294e98933bd4c1e76ed/clang/lib/CodeGen/BackendUtil.cpp#L1173

clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp
22

There's an -O0 RUN line below where the assume(true) won't be removed.
Updated comments.

clang/test/CodeGenCXX/thinlto_public_type_test_distributed.ll
24

unintentional, fixed. I've changed everything to check 0.preopt since 1.promote wasn't appearing in some cases, not sure why

lld/test/ELF/lto/update_public_type_test.ll
2

done

llvm/lib/LTO/LTOBackend.cpp
597

exposed the existing hasWholeProgramVisibility

llvm/lib/Transforms/IPO/LowerTypeTests.cpp
1807

added a check

the assert that there are no public.type.tests in LTT fails on CodeGenCXX/thinlto-distributed-type-metadata.cpp. for some reason clang doesn't go through the LTO API at [1], it just ends up calling the normal optimization pipeline. any ideas why?

[1] https://github.com/llvm/llvm-project/blob/0c1b32717bcffcf8edf95294e98933bd4c1e76ed/clang/lib/CodeGen/BackendUtil.cpp#L1173

Ah ok, that handling is for some cases where we may decide not to do ThinLTO, but rather compile the IR down to native code normally without a thin link (we use that feature for distributed build system reasons). Looks like we need a fallback mechanism to convert these in that case. Maybe right at the start of DevirtModule::run? We could do in LTT where you are currently asserting too, but to me it seems more natural to get rid of these at the start of WPD, because with LTO they are gone by then.

llvm/lib/LTO/LTOBackend.cpp
597

See note about needing this for legacy LTO API (ThinLTOCodeGenerator.cpp).

llvm/test/LTO/X86/public-type-test.ll
2

Needs some CHECKs?

aeubanks updated this revision to Diff 446566.Thu, Jul 21, 10:27 AM
aeubanks marked an inline comment as done.

update

the assert that there are no public.type.tests in LTT fails on CodeGenCXX/thinlto-distributed-type-metadata.cpp. for some reason clang doesn't go through the LTO API at [1], it just ends up calling the normal optimization pipeline. any ideas why?

[1] https://github.com/llvm/llvm-project/blob/0c1b32717bcffcf8edf95294e98933bd4c1e76ed/clang/lib/CodeGen/BackendUtil.cpp#L1173

Ah ok, that handling is for some cases where we may decide not to do ThinLTO, but rather compile the IR down to native code normally without a thin link (we use that feature for distributed build system reasons). Looks like we need a fallback mechanism to convert these in that case. Maybe right at the start of DevirtModule::run? We could do in LTT where you are currently asserting too, but to me it seems more natural to get rid of these at the start of WPD, because with LTO they are gone by then.

I went back to my previous implementation of doing it LTT since we're dropping normal type tests there anyway, and added a comment about this

llvm/lib/LTO/LTOBackend.cpp
597

Added both updatePublicTypeTestCalls and setWithWholeProgramVisibility to ThinLTOCodeGenerator.cpp, which public-type-test.ll tests

tejohnson added inline comments.Fri, Jul 22, 9:46 AM
llvm/lib/LTO/ThinLTOCodeGenerator.cpp
456

Add a comment to see note at call to updateVCallVisibilityInIndex for why parmeter is set to false.

llvm/test/LTO/X86/public-type-test.ll
4

The new version switched this from a regular LTO to ThinLTO test. I think we need both. Can you move the new version into llvm/test/ThinLTO/X86 and make this version back to testing the regular LTO old API handling, but with checks?

aeubanks updated this revision to Diff 447170.Sun, Jul 24, 6:57 PM
aeubanks marked an inline comment as done.

update

random question, if the old API is "legacy", are there any plans to remove it?

llvm/test/LTO/X86/public-type-test.ll
4

done, had to add a new flag to dump IR right before optimizations

random question, if the old API is "legacy", are there any plans to remove it?

@fhahn started to work on this at some point (see https://bugs.llvm.org/show_bug.cgi?id=41541), but I'm not sure of the status. It is used by ld64 and I believe the sony toolchain too.

lld/test/ELF/lto/update_public_type_test.ll
6

Just realized there isn't any testing of the regular LTO handling with the new LTO API. Can you extend this test to check regular LTO as well?

llvm/test/LTO/X86/public-type-test.ll
4

I see, looks like llvm-lto has a thinlto-save-temps that is wired up to dump the same outputs as the new LTO API's save temps, but regular LTO didn't have the same support. I'd go ahead and use the same naming convention (0.preopt.bc) for compatibility with the new LTO API, in case that save temps handling is ever generalized to support both thin and regular LTO.

aeubanks marked an inline comment as done.Mon, Jul 25, 8:48 AM

random question, if the old API is "legacy", are there any plans to remove it?

@fhahn started to work on this at some point (see https://bugs.llvm.org/show_bug.cgi?id=41541), but I'm not sure of the status. It is used by ld64 and I believe the sony toolchain too.

if I'm seeing correctly, looks like there aren't any in-tree users of ThinLTOCodeGenerator/LTOCodeGenerator. are those out of tree?

lld/test/ELF/lto/update_public_type_test.ll
6

extended llvm/test/LTO/X86/public-type-test.ll

tejohnson accepted this revision.Tue, Jul 26, 6:48 AM

lgtm

random question, if the old API is "legacy", are there any plans to remove it?

@fhahn started to work on this at some point (see https://bugs.llvm.org/show_bug.cgi?id=41541), but I'm not sure of the status. It is used by ld64 and I believe the sony toolchain too.

if I'm seeing correctly, looks like there aren't any in-tree users of ThinLTOCodeGenerator/LTOCodeGenerator. are those out of tree?

Yes, all linker users are out of tree. Only llvm-lto is in tree for testing.

This revision is now accepted and ready to land.Tue, Jul 26, 6:48 AM

thanks for the review!

This revision was landed with ongoing or failed builds.Tue, Jul 26, 8:02 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Tue, Jul 26, 9:18 AM

lgtm

random question, if the old API is "legacy", are there any plans to remove it?

@fhahn started to work on this at some point (see https://bugs.llvm.org/show_bug.cgi?id=41541), but I'm not sure of the status. It is used by ld64 and I believe the sony toolchain too.

if I'm seeing correctly, looks like there aren't any in-tree users of ThinLTOCodeGenerator/LTOCodeGenerator. are those out of tree?

Yes, all linker users are out of tree. Only llvm-lto is in tree for testing.

AFAIK ld64 uses ThinLTOCodeGenerator/LTOCodeGenerator through libTLO, which is in-tree (llvm/tools/lto/lto.cpp)