Page MenuHomePhabricator

Add SVN version to libLLVMLTO
ClosedPublic

Authored by mehdi_amini on Apr 11 2016, 12:41 PM.

Details

Summary

For Incremental LTO, we need to make sure that an old
cache entry is not used when incrementally re-linking with a new
libLTO.
Adding a global LLVM_REVISION in llvm-config.h would for to
rebuild/relink the world for every "git pull"/"svn update".
So instead only libLTO is made dependent on the VCS and will
be rebuilt (and the dependent binaries relinked, i.e. as of
today: libLTO.dylib and llvm-lto).

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Make available a macro LLVM_REVISION that is populated with the svn/git revision..
mehdi_amini updated this object.
mehdi_amini added a reviewer: beanz.
mehdi_amini added a subscriber: llvm-commits.
silvas requested changes to this revision.Apr 11 2016, 1:05 PM
silvas added a reviewer: silvas.
silvas added a subscriber: silvas.

This is sort of thorny.

The revision number will change every time I update my checkout, and llvm-config.h is pretty much at the root of the header dependency graph, so basically everything will be recompiled (this will e.g. make "incremental" buildbots useless).

On the other hand, if LLVM_REVISION_STRING is not updated on each checkout, then it is not reliable and that is sort of pointless.

This revision now requires changes to proceed.Apr 11 2016, 1:05 PM
mehdi_amini edited edge metadata.

Limit to libLTO to prevent rebuilding the world

mehdi_amini retitled this revision from Make available a macro LLVM_REVISION that is populated with the svn/git revision. to Add SVN version to libLLVMLTO.Apr 11 2016, 3:06 PM
mehdi_amini updated this object.
mehdi_amini edited edge metadata.

Could you clarify the relation of this with the SVNVersion.inc code in clang? Did you hoist bring it up and share it, does that even make sense, etc.

SVNVersion.inc in clang is partially redundant: it includes both the LLVM revision and the clang revision, it will also trigger a rebuild of libclangBasic and a relink of clang binaries that depends on it.
However SVNVersion.inc only includes the SVN rev, while here we handle either svn or git rev, reusing the LLVM_APPEND_VC_REV scripts.

Okay, this makes sense to me then. Seems reasonable to limit this to libLTO.

beanz accepted this revision.Apr 13 2016, 2:21 PM
beanz edited edge metadata.

This all looks reasonable. As a follow-up it might be nice to refactor some of the related clang infrastructure to reduce duplication of functionality, but that shouldn't block landing this change.

A few minor comments inline.

cmake/modules/VersionFromVCS.cmake
6–10

I assume you'll also have a corresponding clang patch to update the caller there, correct? Alternatively you could have the source_dir be an optional parameter and have it fall back to CMAKE_CURRENT_SOURCE_DIR if ARGN == 0.

lib/LTO/CMakeLists.txt
72

Can you name this something that identifies that it is just for libLTO (i.e. LTORevision.h)?

mehdi_amini marked 2 inline comments as done.
mehdi_amini updated this object.
mehdi_amini edited edge metadata.

Address comments

Thanks, updated. I'll commit later.

cmake/modules/VersionFromVCS.cmake
6–10

Let's go for the optional parameter!

mehdi_amini accepted this revision.Apr 16 2016, 12:39 AM
mehdi_amini accepted this revision.
mehdi_amini added a reviewer: mehdi_amini.
mehdi_amini removed a reviewer: silvas.
This revision is now accepted and ready to land.Apr 16 2016, 12:39 AM
mgorny added a subscriber: mgorny.Oct 20 2016, 12:49 AM

@mehdi_amini, @beanz, I'm sorry for reviving the topic but I can't find the rationale I'm looking for. Am I missing something, but what is the relation between the two new CMake modules (GenerateVersionFromCVS and VersionFromVCS) and the GetSVN module that's there since 2013 (with updates in 2014)? Both seem to revolve around the same code, with little differences. Should other projects be updated to use GenerateVersionFromCVS instead of GetSVN?

@mgorny, as I said in my comment:

As a follow-up it might be nice to refactor some of the related clang infrastructure to reduce duplication of functionality...

We now have 3 separate implementations doing basically the same thing. We really should spend some time cleaning up how these are used in LLVM and the sub-projects. We should be able to get down to a single standard implementation, we're just not anywhere near there.