On Thu, 16 Jan 2025 11:28:55 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
> HotSpot uses precompiled headers to speed up non-incremental builds. The
> current mechanism requires all .cpp files to include precompiled.hpp as the
> first non-comment line. This requires HotSpot devs to think about precompiled
> headers when they create new files and when they need to update/manage the
> include sections.
>
> All the compilers that we use support using precompiled headers without
> explicitly including the associated header in the source code. I propose that
> we use that capability instead.
>
> The make files changes to support this are:
> 1) Remove share/precompiled/ from the list of include directories,
> 2) Compiler-specific changes to support this:
>
> *Windows cl*:
> * Add the share/precompiled/ include directory when generating the PCH file.
> * Use /FI to indicate that we should include precompiled.hpp. /Yu says that
> it should use a PCH for precompiled.hpp. /Fp tells the compiler that it
> should pick up the named precompiled header.
>
> From experiments it seems like it doesn't matter what name I pass in to /FI
> and /Yu, they just have to be the same and the specified file doesn't even
> have to exist, as long as we also pass in the /Fp flag we get the benefits of
> the precompiled header.
>
> *gcc*:
> * Use -include to include the precompiled.hpp file. Note that the compilation
> command line will not have share/precompiled in the include path and that the
> precompiled header will still be picked up correctly.
>
> *clang*:
> * No changes needed, the -include-pch line is all that we need.
>
> I've verified that these changes give the expected compilation time
> reductions both by comparing compilation times in our CI but also by running
> individual compilations of .cpp files on Linux, Mac, and Windows.
>
> Note that the compiler will complain if someone now tries to include
> precompiled.hpp explicitly.
>
> Note also that we have a section about precompiled.hpp in the HotSpot style
> guide. If this proposal is accepted I would like to update the style guide as
> a separate RFE.
>
> Tested by letting GHA build and running tier1-2 in our CI pipeline.
Looks good to me, but would suggest breaking up some of those lines.
make/common/native/CompileFile.gmk line 256:
> 254: FILE := $$($1_GENERATED_PCH_SRC), \
> 255: BASE := $1, \
> 256: EXTRA_CXXFLAGS := -I$$(dir $$($1_PRECOMPILED_HEADER))
> -Fp$$($1_PCH_FILE) -Yc$$(notdir $$($1_PRECOMPILED_HEADER)), \
Suggestion:
EXTRA_CXXFLAGS := -I$$(dir $$($1_PRECOMPILED_HEADER))
-Fp$$($1_PCH_FILE) \
-Yc$$(notdir $$($1_PRECOMPILED_HEADER)), \
make/common/native/CompileFile.gmk line 260:
> 258:
> 259: $1_USE_PCH_FLAGS := \
> 260: -FI$$(notdir $$($1_PRECOMPILED_HEADER)) -Fp$$($1_PCH_FILE)
> -Yu$$(notdir $$($1_PRECOMPILED_HEADER))
Suggestion:
-FI$$(notdir $$($1_PRECOMPILED_HEADER)) -Fp$$($1_PCH_FILE) \
-Yu$$(notdir $$($1_PRECOMPILED_HEADER))
make/common/native/CompileFile.gmk line 274:
> 272: ifeq ($(TOOLCHAIN_TYPE), gcc)
> 273: $1_PCH_FILE := $$($1_OBJECT_DIR)/precompiled/$$(notdir
> $$($1_PRECOMPILED_HEADER)).gch
> 274: $1_USE_PCH_FLAGS := -I$$($1_OBJECT_DIR)/precompiled -include
> $$(notdir $$($1_PRECOMPILED_HEADER))
Suggestion:
$1_USE_PCH_FLAGS := -I$$($1_OBJECT_DIR)/precompiled \
-include $$(notdir $$($1_PRECOMPILED_HEADER))
-------------
Marked as reviewed by erikj (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/23146#pullrequestreview-2556319967
PR Review Comment: https://git.openjdk.org/jdk/pull/23146#discussion_r1918652378
PR Review Comment: https://git.openjdk.org/jdk/pull/23146#discussion_r1918655708
PR Review Comment: https://git.openjdk.org/jdk/pull/23146#discussion_r1918656632
