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

Use -fno-builtin-memcmp with gcc #891

Merged
merged 2 commits into from Jan 10, 2017

Conversation

Projects
None yet
4 participants
@lpw25
Contributor

lpw25 commented Nov 4, 2016

The gcc builtin version of memcmp is actually significantly slower than the function implementation in glib:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43052

For this reason gcc disabled its builtin version after 4.5, but for earlier versions of gcc you need to do -fno-builtin-memcmp to turn it off, which is what this PR does.

Show outdated Hide outdated configure
@@ -358,7 +358,8 @@ case "$ccfamily" in
wrn "Consider using GCC version 4.2 or above."
bytecccompopts="-std=gnu99 -O $gcc_warnings";;
gcc-*)
bytecccompopts="-std=gnu99 -O2 -fno-strict-aliasing -fwrapv $gcc_warnings";;
bytecccompopts="-std=gnu99 -O2 -fno-strict-aliasing -fwrapv \
-fno-builtin-memcmp $gcc_warnings";;

This comment has been minimized.

@avsm

avsm Nov 4, 2016

Member

Is there a minimum version check on gcc before activating this? I believe the -fno-builtin-* variant was only added in GCC 3.1 or thereabouts (previously it was a blanket -fno-builtin to turn it all on or off.

@avsm

avsm Nov 4, 2016

Member

Is there a minimum version check on gcc before activating this? I believe the -fno-builtin-* variant was only added in GCC 3.1 or thereabouts (previously it was a blanket -fno-builtin to turn it all on or off.

This comment has been minimized.

@mshinwell

mshinwell Nov 4, 2016

Contributor

There is a minimum check---see the previous case.

@mshinwell

mshinwell Nov 4, 2016

Contributor

There is a minimum check---see the previous case.

This comment has been minimized.

@avsm

avsm Nov 4, 2016

Member

Ah yes, sorry -- lack of context in web view!

@avsm

avsm Nov 4, 2016

Member

Ah yes, sorry -- lack of context in web view!

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Nov 4, 2016

Contributor

On a benchmark extracted from real code (I think it was s-expression conversion) we found that the glibc version of memcmp outperformed the gcc inlined version on x86-64 by a factor of nearly two. @trefis has the details.

Contributor

mshinwell commented Nov 4, 2016

On a benchmark extracted from real code (I think it was s-expression conversion) we found that the glibc version of memcmp outperformed the gcc inlined version on x86-64 by a factor of nearly two. @trefis has the details.

@gasche gasche added the approved label Nov 4, 2016

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Dec 9, 2016

Contributor

Rebased to trunk. There was a conflict so it might be worth people re-reviewing the configure change.

Contributor

lpw25 commented Dec 9, 2016

Rebased to trunk. There was a conflict so it might be worth people re-reviewing the configure change.

@lpw25 lpw25 merged commit 69880ef into ocaml:trunk Jan 10, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #891 from lpw25/fno-builtin-memcmp
Use -fno-builtin-memcmp with gcc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment