Page MenuHomePhabricator

[ThinLTO/CFI] Include TYPE_ID summaries into GLOBALVAL_SUMMARY_BLOCK
ClosedPublic

Authored by vitalybuka on Jan 26 2018, 4:50 PM.

Diff Detail

Repository
rC Clang

Event Timeline

vitalybuka created this revision.Jan 26 2018, 4:50 PM
pcc added a comment.Feb 6 2018, 5:43 PM

This needs more tests. In particular you should have:

  1. More test coverage for serialization.
  2. A test for the bitcode reader functionality. This should probably be in the form of a test that simulates a distributed build and shows that the correct resolution is passed to the backend. There are a few examples in clang/test/CodeGen/thinlto-* but they all use the legacy LTO API driver (llvm-lto) which is incompatible with ThinLTO+CFI, so you will probably need to have your test use llvm-lto2 (with the -thinlto-distributed-indexes flag).

added tests without devirtualization

removed -mconstructor-aliases flag in tests

pcc added inline comments.Feb 12 2018, 3:08 PM
clang/test/CodeGen/thinlto-distributed-cfi-devirt.cpp
7 ↗(On Diff #133939)

I wonder whether you can simplify this test by making this a .ll file and using opt -thinlto-bc to create the .o file.

72 ↗(On Diff #133939)

I think this will only work on Linux. Same goes for your other tests.

74 ↗(On Diff #133939)

It may be simpler to test the effect of lowertypetests by passing -emit-llvm to clang and examining the IR.

clang/test/CodeGen/thinlto-distributed-cfi.cpp
80 ↗(On Diff #133939)

Since this test and thinlto-distributed-cfi-devirt.cpp use the same inputs, you can merge them into one file.

vitalybuka added inline comments.Feb 12 2018, 4:18 PM
clang/test/CodeGen/thinlto-distributed-cfi-devirt.cpp
7 ↗(On Diff #133939)

I tried tried that from beginning but failed.
Simple dump of %t.o after passing to -thinlto-bc does not produces similar <TYPE_ID> in summaries

72 ↗(On Diff #133939)

Would you prefer if I just remove linking or limit test to linux?

vitalybuka marked 3 inline comments as done.Feb 12 2018, 4:45 PM

updates for comments

fixed typo and added dependency on llvm-lto2

c/cpp -> IR

revert debug change

remove unneeded code

more code removed

update comments

Harbormaster completed remote builds in B14905: Diff 133986.
pcc added inline comments.Feb 13 2018, 3:24 PM
clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
39–43 ↗(On Diff #133986)

I would move these alongside the IR that is being tested.

58–59 ↗(On Diff #133986)

Remove these.

69 ↗(On Diff #133986)

This intrinsic call looks a little odd if the intent is to test that whole-program devirtualization was not applied. In principle it could be locally devirtualized by loading a function pointer from the constant _ZTV1B, which would make the branch below unconditional. Can you instead load a vtable from a passed-in object and pass that to the intrinsic? Basically, make this look like call2 or call3 in llvm/test/Transforms/WholeProgramDevirt/import.ll. The same goes for the other call below.

It was also a little surprising to me at first that this wasn't virtual-const-propagated, but I guess the reason why it wasn't is that you are passing an argument of type double. A more reliable way of preventing virtual const prop would be to pass undef as one of the arguments (see call2 in the same test).

82 ↗(On Diff #133986)

Can you please also include testing for successful devirtualization? I guess you could do that by changing the metadata here to !"_ZTS1B" and testing that the call was successfully single-impl-devirtualized.

clang/test/CodeGen/thinlto-distributed-cfi.ll
27 ↗(On Diff #133986)

You could write -o - here and pipe into FileCheck as usual.

49 ↗(On Diff #133986)

The backend could in principle replace this with true because the first argument is a constant. Could you instead add an argument to main and pass that argument through as the first argument here?

vitalybuka marked 2 inline comments as done.

resolved trivial comments

vitalybuka marked 2 inline comments as done.

add devirtualization checks

vitalybuka marked 2 inline comments as done.Feb 14 2018, 2:14 PM
pcc accepted this revision.Feb 14 2018, 2:25 PM

LGTM

clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
59 ↗(On Diff #134313)

Move this to before line 71.

93 ↗(On Diff #134313)

I think you can remove this main function as well as the one in thinlto-distributed-cfi.ll.

This revision is now accepted and ready to land.Feb 14 2018, 2:25 PM

Remove unneeded main() functions

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
vitalybuka marked an inline comment as done.Feb 14 2018, 2:55 PM
vitalybuka added inline comments.
clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
59 ↗(On Diff #134313)

done in r325184