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

fixed zstd-pgo target for GCC #3281

Merged
merged 2 commits into from
Oct 8, 2022
Merged

fixed zstd-pgo target for GCC #3281

merged 2 commits into from
Oct 8, 2022

Conversation

ilyakurdyukov
Copy link
Contributor

@ilyakurdyukov ilyakurdyukov commented Oct 5, 2022

Since your Makefile uses obj/$(HASH_DIR) for object files, this code does not work correctly for GCC. Because profiles are saved in one directory, and are expected in another when reading.

$(RM) zstd *.o - this line doesn't delete object files.

Clang stores profiles in the current directory, so the problem doesn't appear when compiling with Clang.

Also this code will work if BUILD_DIR is set.

Additional comment from @ldv-alt:

Apparently, zstd-pgo target was broken at least for gcc compiler since the commit that introduced BUILD_DIR for programs subdirectory. Starting with that commit, the object directory used for generating profile data is different from the object directory used for producing the result based on that profile data, and this of course does not work with gcc which uses the object directory to store the profile data. Fortunately, gcc detects this and complains by issuing a warning for each object file, e.g.

../lib/common/zstd_common.c:83:1: warning: '/path/to/zstd/programs/obj/conf_3da26c95555a8e4d4ab71a33da334472/zstd_common.gcda' profile count data file not found [-Wmissing-profile]

Fix this by forwarding HASH_DIR so that the same BUILD_DIR is used both for generating and using profile data.

Since your Makefile uses obj/$(HASH_DIR) for object files, this code does not work correctly for GCC. Because profiles are saved in one directory, and are expected in another when reading.

`$(RM) zstd *.o` - this line doesn't delete object files.

Clang stores profiles in the current directory, so the problem doesn't appear when compiling with Clang.

Also this code will work if BUILD_DIR is set.
Comment on lines 243 to 247
ifndef BUILD_DIR
$(RM) zstd obj/$(HASH_DIR)/*.o
else
$(RM) zstd $(BUILD_DIR)/*.o
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest removing obj/$(HASH_DIR)/zstd and $(BUILD_DIR)/zstd as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ldv-alt
Copy link
Contributor

ldv-alt commented Oct 5, 2022

I also suggest re-using commit message from #3282 which sheds some light on the history of the issue.

Just a precaution, because it works anyway.
@ldv-alt
Copy link
Contributor

ldv-alt commented Oct 7, 2022

I suggest this PR to be merged, this would be the first commit from Ilya into this repository, which in turn would facilitate his patch submissions in the future.

@Cyan4973 Cyan4973 merged commit b63854b into facebook:dev Oct 8, 2022
@Cyan4973
Copy link
Contributor

Cyan4973 commented Oct 8, 2022

Thanks !

@renovate renovate bot mentioned this pull request Feb 10, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants