This is an archive of the discontinued LLVM Phabricator instance.

Do not relink test targets every time in compiler-rt
ClosedPublic

Authored by george.karpenkov on Aug 18 2017, 6:42 PM.

Details

Summary

CMake's add_custom_target is considered to be *always* out of date.
This patch changes it to a combination of add_custom_target and add_custom_command which actually tracks dependencies' timestamps.

On my machine this reliably saves 6-7 seconds on each test group.
This can be a large difference when debugging small tests.

Diff Detail

Repository
rL LLVM

Event Timeline

kcc edited edge metadata.Aug 19 2017, 6:37 PM

Vitaly, plz take a look.
George, please describe how you've tested this change.

Fixed the diff, the previous one was the reverse.

@vitalybuka could you please test with a multi-arch build? I do not have that working on my Linux machine.
@kcc: I have run check-all, check-msan, check-tsan, check-asan on a fresh checkout on Linux and Mac.
I have run check-tsan repeatedly to make sure linking is not reperformed.

I have modified tsan code to make sure test fail on erroneous modifications, and then reverted the change, making sure they do go back to green.

kcc added a comment.Aug 21 2017, 12:50 PM

testing sounds good, thanks!
Still want Vitaly to review.

vitalybuka edited edge metadata.Aug 21 2017, 1:29 PM

check-asan-dynamic does not work for me. however it does not work even on mast without the patch.

Does it work for you?

@vitalybuka it does, but as I've said, I couldn't get multi-architecture build working (my Linux machine produces only 64bit binaries)
Could you tell me at which revision did it stop working? From what I recall, it did use to work after the CMake-refactoring patch.

vitalybuka accepted this revision.Aug 21 2017, 2:13 PM

@vitalybuka it does, but as I've said, I couldn't get multi-architecture build working (my Linux machine produces only 64bit binaries)
Could you tell me at which revision did it stop working? From what I recall, it did use to work after the CMake-refactoring patch.

I am looking into this. Problem is that I've update my linux distro after the time I reviewed your refactoring. Maybe root case is there.
So lets assume we have no evidence that something is wrong with this patch.

This revision is now accepted and ready to land.Aug 21 2017, 2:13 PM
This revision was automatically updated to reflect the committed changes.

check-asan-dynamic works works with lld, with or without your patch. so maybe previously I had newer linker.