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

Fix Compilation on ppc64le using Clang 11 #7713

Closed

Conversation

adamretter
Copy link
Collaborator

Closes #7691

The optimised CRC code for PPC64le which was originally imported in #2353 is not compatible with Clang 11. It looks like the code most likely originated from https://github.com/antonblanchard/crc32-vpmsum.

The code relied on a GCC header file ppc-asm.h which is not available in Clang.

To solve this, I have taken the same approach as the the upstream project from which the CRC code came antonblanchard/crc32-vpmsum@ffc8018#diff-ec3e62c56fbcddeb07230f2a4673c1abd7f0f1cc8e48a2aa560056cfc1b25d60 and simply imported a copy of the GCC header file into our code-base which will be used when Clang is the compiler on pcc64le.

NOTE: The new file util/ppc-asm.h may have licensing implications which I guess need to be approved by RocksDB/Facebook before this is merged

…acebook#2353 is not compatible with Clang 11. The code relies on a GCC header file ppc-asm.h. To solve this, I have taken the same approach as the the upstream project from which the CRC code came antonblanchard/crc32-vpmsum@ffc8018#diff-ec3e62c56fbcddeb07230f2a4673c1abd7f0f1cc8e48a2aa560056cfc1b25d60 and imported a copy of the GCC header file into our code-base which will be used when Clang is the compiler.

NOTE: This may have licensing implications which need to be approved by FB before this is merged.
@adamretter adamretter added ppc64le bug Confirmed RocksDB bugs labels Nov 25, 2020
@adamretter
Copy link
Collaborator Author

I cannot fine how to exclude the file util/ppc-asm.h from the clang format check... does anyone know how?

util/crc32c.cc Outdated
@@ -346,6 +346,7 @@ static inline void Slow_CRC32(uint64_t* l, uint8_t const **p) {
table0_[c >> 24];
}

#if defined HAVE_SSE42 && defined NO_THREEWAY_CRC32C
Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of builds seem broken, probably from this?

util/ppc-asm.h Outdated
Comment on lines 3 to 10
Copyright (C) 2002-2020 Free Software Foundation, Inc.

This file is part of GCC.

GCC is free software; you can redistribute it and/or modify it under
the terms of the GNU General Public License as published by the Free
Software Foundation; either version 3, or (at your option) any later
version.
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be permissible copying. (Internal ref: https://fburl.com/wiki/s1ewaih5)

However, I think it belongs under third-part/gcc/ or even port/ instead of util/.

@pdillinger
Copy link
Contributor

I cannot fine how to exclude the file util/ppc-asm.h from the clang format check... does anyone know how?

There's no standard way. We can just ignore the error in rare cases like this. It will not affect future PRs (unless they edit the affected file)

@adamretter
Copy link
Collaborator Author

adamretter commented Nov 30, 2020

@pdillinger Ah, right! Sorry I forgot to backport a similar fix that I made in the Apple Silicon PR to this branch.
Hopefully it should be good to go now...

Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

Please put ppc-asm.h under third-party/gcc/ or at least port/

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@adamretter has updated the pull request. You must reimport the pull request before landing.

@adamretter
Copy link
Collaborator Author

@pdillinger Done.

Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

👍 👍

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@pdillinger merged this pull request in b937be3.

heckj pushed a commit to heckj/rocksdb that referenced this pull request Jan 9, 2021
Summary:
Closes facebook#7691

The optimised CRC code for PPC64le which was originally imported in facebook#2353 is not compatible with Clang 11. It looks like the code most likely originated from https://github.com/antonblanchard/crc32-vpmsum.

The code relied on a GCC header file `ppc-asm.h` which is not available in Clang.

To solve this, I have taken the same approach as the the upstream project from which the CRC code came antonblanchard/crc32-vpmsum@ffc8018#diff-ec3e62c56fbcddeb07230f2a4673c1abd7f0f1cc8e48a2aa560056cfc1b25d60 and simply imported a copy of the GCC header file into our code-base which will be used when Clang is the compiler on pcc64le.

**NOTE**: The new file `util/ppc-asm.h` may have licensing implications which I guess need to be approved by RocksDB/Facebook before this is merged

Pull Request resolved: facebook#7713

Reviewed By: jay-zhuang

Differential Revision: D25222645

Pulled By: pdillinger

fbshipit-source-id: e3fec9136f26ce1eb7a027048bcf77a6cb3c769c
codingrhythm pushed a commit to SafetyCulture/rocksdb that referenced this pull request Mar 5, 2021
Summary:
Closes facebook#7691

The optimised CRC code for PPC64le which was originally imported in facebook#2353 is not compatible with Clang 11. It looks like the code most likely originated from https://github.com/antonblanchard/crc32-vpmsum.

The code relied on a GCC header file `ppc-asm.h` which is not available in Clang.

To solve this, I have taken the same approach as the the upstream project from which the CRC code came antonblanchard/crc32-vpmsum@ffc8018#diff-ec3e62c56fbcddeb07230f2a4673c1abd7f0f1cc8e48a2aa560056cfc1b25d60 and simply imported a copy of the GCC header file into our code-base which will be used when Clang is the compiler on pcc64le.

**NOTE**: The new file `util/ppc-asm.h` may have licensing implications which I guess need to be approved by RocksDB/Facebook before this is merged

Pull Request resolved: facebook#7713

Reviewed By: jay-zhuang

Differential Revision: D25222645

Pulled By: pdillinger

fbshipit-source-id: e3fec9136f26ce1eb7a027048bcf77a6cb3c769c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build error on powerpc64 with Clang 11.0.0
3 participants