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

Enable PCHs for IR headers #5033

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Enable PCHs for IR headers #5033

wants to merge 5 commits into from

Conversation

asl
Copy link
Contributor

@asl asl commented Nov 25, 2024

Fixes #5032

@asl asl added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Nov 25, 2024
@asl asl requested review from vlstill, ChrisDodd and fruffy November 25, 2024 09:46
@asl
Copy link
Contributor Author

asl commented Nov 25, 2024

Tofino failures are due to inclusion of two C files into a C++ project / library (without proper language specification in the CMake project):

bf-utils/src/dynamic_hash/dynamic_hash.c
bf-utils/src/dynamic_hash/bfn_hash_algorithm.c

Can these be just ported to C++?

@fruffy
Copy link
Collaborator

fruffy commented Nov 25, 2024

Tofino failures are due to inclusion of two C files into a C++ project / library (without proper language specification in the CMake project):

bf-utils/src/dynamic_hash/dynamic_hash.c
bf-utils/src/dynamic_hash/bfn_hash_algorithm.c

Can these be just ported to C++?

Yes! Iirc this used to be a separate library which was added to the compiler during open-sourcing.

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@asl
Copy link
Contributor Author

asl commented Nov 26, 2024

So, there are two issues here:

  1. Tofino backend is using C sources in C++ mode
  2. Somehow PCH generation conflicts with PIE codegen. This seems to depend on the particular cmake version being used as it fails on Ubuntu 20, but fine everywhere else.

Looks like p4c is compiled always with position independent code. This looks like some workaround to me:

# Always build position-independent code. This is important when linking with Protobuf.
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

Does anyone remember why this was needed? Is it still needed? @fruffy

@asl asl added run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. run-static Use this tag to trigger static build CI run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-validation Use this tag to trigger a Validation CI run. labels Nov 26, 2024
@fruffy
Copy link
Collaborator

fruffy commented Nov 26, 2024

Does anyone remember why this was needed? Is it still needed? @fruffy

Iirc if you do not enable this on clang this will cause linking issues with protobuf. I haven't found a better solution than enabling it for all CMake targets.

@asl
Copy link
Contributor Author

asl commented Nov 26, 2024

Iirc if you do not enable this on clang this will cause linking issues with protobuf. I haven't found a better solution than enabling it for all CMake targets.

Looks like this is not needed anymore? I removed this from this PR and everything works fine.

@fruffy
Copy link
Collaborator

fruffy commented Nov 26, 2024

Iirc if you do not enable this on clang this will cause linking issues with protobuf. I haven't found a better solution than enabling it for all CMake targets.

Looks like this is not needed anymore? I removed this from this PR and everything works fine.

Sanitizers is failing. Let's see whether it throws an error after compilation. I would be happy to remove this.

@asl
Copy link
Contributor Author

asl commented Nov 26, 2024

Sanitizers is failing. Let's see whether it throws an error after compilation. I would be happy to remove this.

Yes, need to see what is wrong, but likely unrelated.

@asl
Copy link
Contributor Author

asl commented Nov 26, 2024

@fruffy clang bug wrt __builtin_bit_cast and PCH: llvm/llvm-project@95d7ccb

It seems to be fixed in clang 11. The CI is using very old clang 10. Is it possible to upgrade? :)

@fruffy
Copy link
Collaborator

fruffy commented Nov 26, 2024

@fruffy clang bug wrt __builtin_bit_cast and PCH: llvm/llvm-project@95d7ccb

It seems to be fixed in clang 11. The CI is using very old clang 10. Is it possible to upgrade? :)

Unfortunately Ubuntu 20.04 uses clang-10 by default. As long as we are committed to supporting the system this won't be possible. Unless we raise the minimum version to 11/12 and demand that users install that version.

This is an issue that should be raised separately.

@fruffy
Copy link
Collaborator

fruffy commented Nov 26, 2024

Alternatively maybe we can simply patch Abseil for this version?

@asl
Copy link
Contributor Author

asl commented Nov 27, 2024

Actually, I think we need to discuss if we'd want to enable PCH by default:

  • It definitely speeds up clean rebuild
  • However, it interferes with ccache or other caches – precompiled header essentially makes everything uncached

So, for small changes (not affecting IR, for example) PCH would yield longer compile times as compared to ccache. For larger changes PCHs seem to be fine.

@asl
Copy link
Contributor Author

asl commented Dec 2, 2024

So, I disabled PCHs by default for now. Looks like there are some tradeoffs wrt ccache, so one needs to decide what might be preferable.

Alternatively, we can have PCHs enabled by default if there is no ccache support.

@@ -7,10 +7,6 @@
#include <string.h>
#endif

#ifdef __cplusplus
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we split these changes into a separate PR?

@fruffy
Copy link
Collaborator

fruffy commented Dec 2, 2024

So, I disabled PCHs by default for now. Looks like there are some tradeoffs wrt ccache, so one needs to decide what might be preferable.

Alternatively, we can have PCHs enabled by default if there is no ccache support.

Thanks for the work! I am actually a little confused why we have this conflict with ccache. Shouldn't the headers stay the same?

It looks like there are some options for the headers: https://ccache.dev/manual/3.2.4.html#_precompiled_headers

@asl
Copy link
Contributor Author

asl commented Dec 2, 2024

Thanks for the work! I am actually a little confused why we have this conflict with ccache. Shouldn't the headers stay the same?

Headers are the same, indeed. However, ccache tracks also:

  • command line (so to ensure that options did not change)
  • the contents of all the files involved (including the PCH)

In general, PCH contents could be different even if the files did not change, and as a result, ccache might decide a full recompile is necessary. There are some options to relax some PCH checks, but these should be done on the client, not on the project side.

asl added 3 commits December 3, 2024 09:40
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
…IE always enable (do we really need it though?)

Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
asl added 2 commits December 3, 2024 09:40
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. run-validation Use this tag to trigger a Validation CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate use of precompiled headers
3 participants