Page MenuHomePhabricator

IRGen: Start using the WriteThinLTOBitcode pass.
ClosedPublic

Authored by pcc on Jan 17 2017, 7:08 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Jan 17 2017, 7:08 PM
tejohnson edited edge metadata.Jan 17 2017, 10:04 PM

TODO: avoid breaking Darwin.

How does this break Darwin?

clang/lib/CodeGen/BackendUtil.cpp
694 ↗(On Diff #84792)

Can we transform other callers of createBitcodeWriterPass with EmitSummaryIndex = true or EmitModuleHash = true to instead call createWriteThinLTOBitcodePass as you did here, and then remove those parameters from createBitcodeWriterPass?

clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

Is it the case that now we will always split the module with this change? Should that only be done under CFI options?

mehdi_amini edited edge metadata.Jan 17 2017, 10:30 PM

TODO: avoid breaking Darwin.

How does this break Darwin?

IIUC, we don't use the new LTO API, which is handling this gracefully.

clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

Devirtualization may happen whenever you have a hidden virtual table IIUC, independently of CFI.

pcc added inline comments.Jan 18 2017, 1:02 PM
clang/lib/CodeGen/BackendUtil.cpp
694 ↗(On Diff #84792)

http://llvm-cs.pcc.me.uk/lib/Bitcode/Writer/BitcodeWriterPass.cpp/rcreateBitcodeWriterPass

The only other caller like that is in opt. I think that if we change the "under the hood" parts of opt we should change it to not use a pass at all (c.f. our earlier discussion on D27324), then remove those parameters.

clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

To be more precise: we normally add type metadata in LTO mode when the class has hidden visibility. See: http://clang.llvm.org/docs/LTOVisibility.html

That doesn't necessarily imply devirtualization, which is controlled by the flag -fwhole-program-vtables.

mehdi_amini added inline comments.Jan 18 2017, 1:19 PM
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

So with hidden visibility but without CFI or -fwhole-program-vtables, do we split the module? What's the purpose?

pcc added inline comments.Jan 18 2017, 1:32 PM
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

At the moment we would. The purpose is to simplify the overall interface. If I want to compile a subset of my TUs without CFI or devirtualization, I should be able to do that by enabling LTO but not passing the CFI or devirtualization flags. In that case the vtables themselves should still have type metadata so that TUs compiled without CFI/devirtualization can correctly interoperate with TUs compiled with CFI/devirtualization (to handle the cases where a class defined in a TU compiled without CFI/devirt is used by code compiled with LTO/devirt, or where the linker/LTO selects a linkonce_odr vtable from a TU compiled without CFI/devirt).

I'd be open to changing the command line interface so that an additional flag may be used to control the scope of the "LTO unit" and which would just enable type metadata, but ideally I'd like to keep things relatively simple.

mehdi_amini added inline comments.Jan 18 2017, 1:38 PM
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

At the moment we would. The purpose is to simplify the overall interface.

Right, if it was the only reason, I wouldn't be in favor, but you raise a real use case below.

If I want to compile a subset of my TUs without CFI or devirtualization, I should be able to do that by enabling LTO but not passing the CFI or devirtualization flags.

Right, seems legit.

In that case the vtables themselves should still have type metadata so that TUs compiled without CFI/devirtualization can correctly interoperate with TUs compiled with CFI/devirtualization

That's what I wasn't sure about :)

(to handle the cases where a class defined in a TU compiled without CFI/devirt is used by code compiled with LTO/devirt, or where the linker/LTO selects a linkonce_odr vtable from a TU compiled without CFI/devirt).

Make sense, LGTM as is with this explanation!
Thanks.

tejohnson added inline comments.Jan 18 2017, 1:40 PM
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

I would like some way to disable this, at least for debugging.

mehdi_amini added inline comments.Jan 18 2017, 1:46 PM
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

CC1 option then?

tejohnson added inline comments.Jan 18 2017, 2:00 PM
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)

CC1 option then?

Sure

pcc added inline comments.Jan 18 2017, 3:58 PM
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
2 ↗(On Diff #84792)
pcc edited the summary of this revision. (Show Details)Jan 18 2017, 4:07 PM
pcc updated this revision to Diff 84903.Jan 18 2017, 4:07 PM

Refresh

This revision is now accepted and ready to land.Jan 18 2017, 4:10 PM
pcc updated this revision to Diff 84907.Jan 18 2017, 4:17 PM
  • Add missing test dependency
This revision was automatically updated to reflect the committed changes.