-
Notifications
You must be signed in to change notification settings - Fork 100
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
#0: Faster builds by enabling Unity build for TTNN and tests #14461
Conversation
@afuller-TT I'm good with the suggestions, but we should plan to keep an eye on the build artifact times over a period. (which is what you said)
I agree.
Faster than a build without ccache, yes, but then why would the rebuild of a few merged .cpp files (and remember, given our software size, these are probably going to be large TUs) be faster than re-building only the affected TUs with the others cached? Wouldn't we be compiling either an equal amount or more stuff fundamentally upon a ccache miss with unity build on, as opposed to ccache miss with unity build off? And sure, we would have likely <N cache misses with unity build on, but these TUs are still quite large, so the cache entries are large. Re-building smaller parts of it via individual TUs rather than a larger merged one with files that may have no changes should take less time, regardless of how many less cache misses we have. As a thought experiment, what happens if we were to have only one giant TU? (obviously not the case but I'm just saying) Again, given @afuller-TT 's response above, I think it's still worth trying to see what the effect is on main. But I still am having trouble understanding your last point. |
@tt-rkim
I also agree, let's just wait and see what's going to happen in practice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go! 🚀
@@ -186,6 +190,7 @@ bool writer_kernel_no_receive( | |||
} | |||
|
|||
TEST_F(N300DeviceFixture, EthKernelsNocReadNoSend) { | |||
using namespace CMAKE_UNIQUE_NAMESPACE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are kinda noisy and not related to the tests themselves. Does Unity trip up if we have it at file-scope instead of f'n scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Let's say we have two files which are being merged in unity build, and both of them define the same function within CMAKE_UNIQUE_NAMESPACE. If both of them do using namespace at the file level, it would result in both functions being visible at the same time and the call would be ambiguous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That makes perfect sense.
@@ -22,12 +22,6 @@ using std::vector; | |||
using namespace tt; | |||
using namespace tt::tt_metal; | |||
|
|||
struct TestBufferConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code removal 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this.
Ticket
Problem description
Currently build times are really slow. After this change, clean build of the whole repo with tests is about 2x faster.
What's changed
Enable Unity build for TTNN and tests
Fix all of the compilation issues, primarily missing
#pragma once
in headers and usage of anonymous namespaces.Future work
According to my observation, build times can be improved significantly by merging a lot of single-cpp test binaries together and applying Unity builds to them. This is considered to be out of scope of this pull request.
Checklist