Open TT-billteng opened 2 months ago
PCH for third party headers have been added. Unfortunately, right now it seems metal doesn't play well with pch due to the different build flags of our targets and rapidly changing header files.
PCH for third party headers have been added. Unfortunately, right now it seems metal doesn't play well with pch due to the different build flags of our targets and rapidly changing header files.
but you submitted to main? Something broken?
right now it seems metal doesn't play well with pch due to the different build flags of our targets and rapidly changing header files.
This doesn't sound right to me. CMake should automatically detect flag changes and recompile the PCH. Also due to how slow TTNN compiles. It should still result in faster built times even just within a single run.
From what I've observed the different build flags prevents us from using the REUSE_FROM
option in target_precompile_headers
.
From my testing with PCHs with ttnn, I've observed slower single run compile times compared to without PCHs using both make
and ninja
. The branch I've been using is vtangTT/testing_pch
, where you can see the headers I'm trying to pre-compile in ttnn/CMakeLists.txt
Please let me know if I'm misunderstanding what you meant in regards to using PCHs in ttnn.
@vtangTT I'm currently dealing with some family affairs. I'll get back to you as soon as I'm done with it.
I think REUSE_FROM might be the issue. That imports some headers that might not be needed from library A into also compiling for library B. Each library/executable should have their own list of headers (even if it's just string
and fmt
).
@vtangTT Which branch contains your PCH patches? I found vtangTT/testing_pch
is this the one'?
Yes that is the one.
Minor note. It'll be great if you could enable precompiled headers during build. Should cut down the build time by a lot for Metalium. TTNN takes a while to build, I think it'll greatly benefit from it.
Originally posted by @marty1885 in https://github.com/tenstorrent/tt-metal/issues/7914#issuecomment-2097463664