Page MenuHomePhabricator

[ThinLTO/WPD] Remove dead code and improve test case
AbandonedPublic

Authored by evgeny777 on Feb 6 2020, 7:54 AM.

Details

Reviewers
tejohnson
Summary

I was studying changes to BackendUtil introduced by D71913 and found out that

  • Removing invocation of LTT pass in BackendUtil.cpp doesn't break any test case including thinlto-distributed-type-metadata.cpp
  • When combined index doesn't have skipModuleByDistributedBackendflag we run normal thin LTO pipeline which removes type.test/assume sequences either in WPD or in LTT.
  • When combined index does have skipModuleByDistributedBackend flag we end up with empty module without invoking backend.

So adding LTT pass in BackendUtil seems to be not needed.

Diff Detail

Event Timeline

evgeny777 created this revision.Feb 6 2020, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2020, 7:54 AM

Let me investigate and get back. I know I ran into issues when testing our apps with my changes that required this. But perhaps it is only the invocation at line 1180 - are you saying that one is needed but the others aren't?

I know I ran into issues when testing our apps with my changes that required this. But perhaps it is only the invocation at line 1180 - are you saying that one is needed but the others aren't?

I'm not sure it's needed as well. Most likely I simply missed it.
If I'm wrong about those invocations please submit/modify the test case.

I know I ran into issues when testing our apps with my changes that required this. But perhaps it is only the invocation at line 1180 - are you saying that one is needed but the others aren't?

I'm not sure it's needed as well. Most likely I simply missed it.
If I'm wrong about those invocations please submit/modify the test case.

I had added some cases to clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp for these:

// Also check type test are lowered when the distributed ThinLTO backend clang
// invocation is passed an empty index file, in which case a non-ThinLTO
// compilation pipeline is invoked. If not lowered then LLVM CodeGen may assert.

If I remove the third LTT call in BackendUtil.cpp (corresponding to the new PM >O0), then it gets an assert:

Can't get register for value!
UNREACHABLE executed at /usr/local/google/home/tejohnson/llvm/llvm_m1/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1610!

I did also add invocations of the test there for the other paths (old PM, and newPM -O0), but apparently they don't hit the same assert. Even though I haven't been able to trigger the same failure in those cases, I think it makes sense to keep these paths consistent in terms of the code they will send through the LLVM pipeline.

Note that this case is different than what the skipModuleByDistributedBackend flag is indicating. That flag indicates that the linker decided not to include any symbols from the module. The case this is handling is when we have a module, but an empty index file (it was /dev/null). This is used in our distributed build in some cases where we want to compile the bitcode down to native code without applying any LTO optimizations (in certain cases for testing, or a few situations where ThinLTO isn't well supported). In that case there is no WPD/LTT as we do a non-ThinLTO pipeline.

evgeny777 abandoned this revision.Feb 6 2020, 10:47 PM

Thanks!