Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CMake] Split out libinfo.cc into a separate target. #8520

Merged
merged 1 commit into from
Jul 25, 2021

Conversation

Lunderberg
Copy link
Contributor

Every *.o file in the cmake-generated makefiles have a dependency on the target's flags.make file. The flags.make file contains the compiler flags for all objects in a target, not just the *.o file currently being compiled. As a result, even though libinfo.cc is the only file that has the TVM_GIT_COMMIT_TIME and TVM_GIT_COMMIT_HASH definitions, every file in the tvm_objs target was recompiled whenever the commit id was changed.

By splitting libinfo.cc out into a separate target, no other files need to be recompiled when committing or changing branches, unless there are actual changes to a file.

@Lunderberg
Copy link
Contributor Author

@junrushao1994 Could I get a review from you, since you originally implemented the libinfo.cc functionality? From testing with tvm.support.libinfo(), I don't see any changes or breaking of functionality.

Copy link
Member

@jroesch jroesch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me if everything is green, I had complained that something was building too often, thanks so much for figuring it out!

Copy link
Member

@junrushao junrushao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Lunderberg for fixing this problem! I didn't know this issue before :-)

@junrushao
Copy link
Member

CC @comaniac who added the TVM_GIT_COMMIT_TIME :-)

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@junrushao
Copy link
Member

junrushao commented Jul 22, 2021

Not in the scope of this PR, I am thinking if we should add a cmake target for each subfolder like src/runtime, src/tir, src/relay and each target could have their own precompiled header, unity build, etc.

@Lunderberg
Copy link
Contributor Author

Not in the scope of this PR, I am thinking if we should add a cmake target for each subfolder like src/runtime, src/tir, src/relay and each target could have their own precompiled header, unity build, etc.

I think that would make a lot of sense. I could also imagine having a debug option for each target to generate a separate shared library. At least one my computer, linking the final libtvm.so is what takes the majority of the total time if I've only changed a few files. Having a single distributable file is good for deployment, but having a debug libtvm.so that itself points to a dozen smaller libraries would be much faster for incremental testing.

Every `*.o` file in the cmake-generated makefiles have a dependency on
the target's `flags.make` file.  The `flags.make` file contains the
compiler flags for all objects in a target, not just the `*.o` file
currently being compiled.  As a result, even though `libinfo.cc` is
the only file that has the `TVM_GIT_COMMIT_TIME` and
`TVM_GIT_COMMIT_HASH` definitions, every file in the `tvm_objs` target
was recompiled whenever the commit id was changed.

By splitting `libinfo.cc` out into a separate target, no other files
need to be recompiled when committing or changing branches, unless
there are actual changes to the file.
@tqchen tqchen merged commit 18171e4 into apache:main Jul 25, 2021
@Lunderberg Lunderberg deleted the cmake_libinfo branch July 26, 2021 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants