#13787: Fix build with CMAKE_DISABLE_PRECOMPILE_HEADERS #13700
Conversation
afuller-TT
requested review from
cfjchu,
ayerofieiev-tt,
dmakoviichuk-tt,
TT-BrianLiu,
yan-zaretskiy,
SeanNijjar,
ntarafdar,
sjameelTT,
patrickroberts,
eyonland,
jaykru-tt,
mywoodstock,
shwetankTT,
sankarmanoj-tt,
pavlejosipovic,
abhullar-tt,
pgkeller,
aliuTT,
tt-aho,
tt-dma,
tt-asaigal and
ubcheema
as code owners
| @@ -807,6 +807,7 @@ struct get_first_object_of_type_t<std::vector<T>> { | |||
| for (auto& tensor : value) { | |||
| return get_first_object_of_type<object_t>(tensor); | |||
| } | |||
| throw std::out_of_range("No such element"); | |||
There was a problem hiding this comment.
Picked something arbitrary to satisfy the compiler's complaint that the f'n may exit without returning a value. Looking to the owners for context re: are we guarnateed that this will never happen, and can we hint to the compiler as much? Failing that, what is the desired outcome of this f'n?
blozano-tt
force-pushed
the
afuller/fix-missing-headers
branch
from
8a84a9f to
3e6749c
Compare
|
@afuller-TT Do you mind putting the issue number with a colon in your final squashed commit message or edit your commits now, as enforced by git hooks? |
ttnn/cpp/pybind11/events.cpp
Outdated
| @@ -5,6 +5,8 @@ | |||
| #include "events.hpp" | |||
|
|
|||
| #include "tt_metal/impl/event/event.hpp" | |||
| #include "pybind11/operators.h" | |||
There was a problem hiding this comment.
not sure this is the right include here. do you know whats missing?
There was a problem hiding this comment.
Thanks for looking it over. Indeed that's the wrong one. I think I got trigger happy :)
This file was complaining about unable to resolve py::class_ and module.def. Looks like pybind11/pybind11.h is probably the right include. Updated.
blozano-tt
changed the title
|
From @ayerofieiev-tt
Good call out. But fear not, the reason I discovered these is precisely because I'm preparing a build that cannot use PCH, so this is a pre-req and shortly after we get this in we'll be having a build that will detect any regression here. |
|
I'm going to go ahead and merge this without waiting on the final approvals. It's had a number of eyes, including on the one code change. The rest is just #includes. |
### Ticket closes #13787 needed for #13698 ### Problem description When building with PCH disabled (set `CMAKE_DISABLE_PRECOMPILE_HEADERS` to True), the build fails because not all files include the headers they use. They are only accidentally able to resolve certain dependencies. This is a problem, because the first cut of `ccache` is going to skip PCH. Also analysis tools (including the compiler) don't process non-included headers. As evidenced by the one non-include change in this PR that the compiler is now detecting. ### What's changed * Build with `CMAKE_DISABLE_PRECOMPILE_HEADERS = true` * Fix failure * GOTO step 1 ### Checklist - [x] [Post commit CI passes](https://github.com/tenstorrent/tt-metal/actions/runs/11335836891) - [ ] Blackhole Post commit (if applicable) - [ ] Model regression CI testing passes (if applicable) - [ ] Device performance regression CI testing passes (if applicable) - [x] New/Existing tests provide coverage for changes https://github.com/tenstorrent/tt-metal/actions/runs/11335836891 --------- Co-authored-by: Bryan Wilder Field Lozano <[email protected]>
…enstorrent#13700) ### Ticket closes tenstorrent#13787 needed for tenstorrent#13698 ### Problem description When building with PCH disabled (set `CMAKE_DISABLE_PRECOMPILE_HEADERS` to True), the build fails because not all files include the headers they use. They are only accidentally able to resolve certain dependencies. This is a problem, because the first cut of `ccache` is going to skip PCH. Also analysis tools (including the compiler) don't process non-included headers. As evidenced by the one non-include change in this PR that the compiler is now detecting. ### What's changed * Build with `CMAKE_DISABLE_PRECOMPILE_HEADERS = true` * Fix failure * GOTO step 1 ### Checklist - [x] [Post commit CI passes](https://github.com/tenstorrent/tt-metal/actions/runs/11335836891) - [ ] Blackhole Post commit (if applicable) - [ ] Model regression CI testing passes (if applicable) - [ ] Device performance regression CI testing passes (if applicable) - [x] New/Existing tests provide coverage for changes https://github.com/tenstorrent/tt-metal/actions/runs/11335836891 --------- Co-authored-by: Bryan Wilder Field Lozano <[email protected]>
Ticket
closes #13787
needed for #13698
Problem description
When building with PCH disabled (set
CMAKE_DISABLE_PRECOMPILE_HEADERSto True), the build fails because not all files include the headers they use. They are only accidentally able to resolve certain dependencies.This is a problem, because the first cut of
ccacheis going to skip PCH. Also analysis tools (including the compiler) don't process non-included headers. As evidenced by the one non-include change in this PR that the compiler is now detecting.What's changed
CMAKE_DISABLE_PRECOMPILE_HEADERS = trueChecklist
https://github.com/tenstorrent/tt-metal/actions/runs/11335836891