gh-Cyan4973/xxHash
Watch
1
0
Fork
You've already forked xxHash
 0
Code Issues 7 Pull requests 3 Releases 26 Wiki Activity

Pb when building with gcc-12 -Og #720

Closed
Mingli-Yu wants to merge 1 commit from dev into dev
pull from: dev
merge into: gh-Cyan4973:dev
Conversation 11 Commits 1 Files changed 1 +1 -1
Mingli-Yu commented 2022-06-08 03:48:01 -05:00 (Migrated from github.com)

Fixes:
| xxhash.h:3932:1: error: inlining failed in call to 'always_inline' 'XXH3_accumulate_512_sse2': function not considered for inlining
| 3932 | XXH3_accumulate_512_sse2( void* XXH_RESTRICT acc,
| | ^~~~~~~~~~~~~~~~~~~~~~~~
| xxhash.h:4369:9: note: called from here
| 4369 | f_acc512(acc,
| | ^~~~~~~~~~~~~
| 4370 | in,
| | ~~~
| 4371 | secret + n*XXH_SECRET_CONSUME_RATE);
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Mingli Yu mingli.yu@windriver.com

Fixes: | xxhash.h:3932:1: error: inlining failed in call to 'always_inline' 'XXH3_accumulate_512_sse2': function not considered for inlining | 3932 | XXH3_accumulate_512_sse2( void* XXH_RESTRICT acc, | | ^~~~~~~~~~~~~~~~~~~~~~~~ | xxhash.h:4369:9: note: called from here | 4369 | f_acc512(acc, | | ^~~~~~~~~~~~~ | 4370 | in, | | ~~~ | 4371 | secret + n*XXH_SECRET_CONSUME_RATE); | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Mingli Yu <mingli.yu@windriver.com>
t-mat commented 2022-06-10 14:26:40 -05:00 (Migrated from github.com)

Hi @Mingli-Yu,
thank you for sending PR. But unfortunately your PR has failed to pass our test. Also I couldn't repro your issue with gcc-12.1.

First of all, please understand that we can't accept your PR for now. Because your PR failed to pass our test. But I also must say that our test is not perfect. So feel free to point out our mistake.

I failed to reproduce your issue with gcc-12.1

I've tried to build xxhsum and make test with gcc-12.1 by the following commands which found at reddit.

cd

# spack is the cleanest package manager BTW.
git clone -c feature.manyFiles=true https://github.com/spack/spack.git
. spack/share/spack/setup-env.sh
spack install gcc@12.1
spack load gcc@12.1
gcc --version
# gcc (Spack GCC) 12.1.0

git clone --depth 1 https://github.com/Cyan4973/xxHash.git
cd xxHash

make
make test
CPPFLAGS="-mavx2 -DXXH_VECTOR=XXH_AVX2" make clean check
# build xxhsum and pass all tests

./xxhsum --version
# xxhsum 0.8.1 by Yann Collet
# compiled as 64-bit x86_64 + AVX2 little endian with GCC 12.1.0

During the build and test process, I don't see any warning and error. Since you've posted specific error message, I think I missed something. But honestly, I have no idea.

I don't understand intention of your PR.

(1) Same question again, but I don't see any warning (as an error) from these __inline__ and __attribue__((always_inline)) in the macro. How can we reproduce your issue?

(2) Why you removed __attribute__((unused))? According to the gcc manual, I think this attribute may help to ease/resolve yor situation. At least it may help our test.

unused
This attribute, attached to a function, means that the function is meant to be possibly unused. GCC does not produce a warning for this function.

Hi @Mingli-Yu, thank you for sending PR. But unfortunately your PR has failed to pass our test. Also I couldn't repro your issue with `gcc-12.1`. First of all, please understand that we can't accept your PR for now. Because your PR failed to pass our test. But I also must say that our test is not perfect. So feel free to point out our mistake. ---- ### I failed to reproduce your issue with `gcc-12.1` I've tried to build `xxhsum` and `make test` with `gcc-12.1` by the following commands [which found at reddit](https://www.reddit.com/r/Ubuntu/comments/v4se5x/comment/ib5taos/). ``` cd # spack is the cleanest package manager BTW. git clone -c feature.manyFiles=true https://github.com/spack/spack.git . spack/share/spack/setup-env.sh spack install gcc@12.1 spack load gcc@12.1 gcc --version # gcc (Spack GCC) 12.1.0 git clone --depth 1 https://github.com/Cyan4973/xxHash.git cd xxHash make make test CPPFLAGS="-mavx2 -DXXH_VECTOR=XXH_AVX2" make clean check # build xxhsum and pass all tests ./xxhsum --version # xxhsum 0.8.1 by Yann Collet # compiled as 64-bit x86_64 + AVX2 little endian with GCC 12.1.0 ``` During the build and test process, I don't see any warning and error. Since you've posted specific error message, I think I missed something. But honestly, I have no idea. --- ### I don't understand intention of your PR. (1) Same question again, but I don't see any warning (as an error) from these `__inline__` and `__attribue__((always_inline))` in the macro. How can we reproduce your issue? (2) Why you removed `__attribute__((unused))`? According to [the `gcc` manual](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes), I think this attribute may help to ease/resolve yor situation. At least it may help our test. > unused > This attribute, attached to a function, means that the function is meant to be possibly unused. GCC does not produce a warning for this function.
👍 1
jrosdahl commented 2022-07-19 06:57:30 -05:00 (Migrated from github.com)

Looks like @Mingli-Yu forgot to mention that the issue only shows up when using -Og.

Looks like @Mingli-Yu forgot to mention that the issue [only shows up when using `-Og`](https://github.com/ccache/ccache/issues/1085#issuecomment-1149026113).
👍 2
t-mat commented 2022-07-19 08:56:27 -05:00 (Migrated from github.com)

Hi @jrosdahl, thanks for the info!
Now I can reproduce this issue with the following command:

CFLAGS="-Og" CC="gcc-12" make clean default  # NG

Since I have no idea so far, I just put some random commands for further investigation.

# gcc-12 -Og
CFLAGS="-Og -no-mavx2"   CC="gcc-12" make clean default  # NG (SSE2)
CFLAGS="-Og -msse2"      CC="gcc-12" make clean default  # NG (SSE2)
CFLAGS="-Og -mavx2"      CC="gcc-12" make clean default  # OK (AVX2)
CFLAGS="-Og -mhaswell"   CC="gcc-12" make clean default  # OK (AVX2)

# gcc-12 -O?
CFLAGS="-O0"             CC="gcc-12" make clean default  # OK (SSE2)
CFLAGS="-O1"             CC="gcc-12" make clean default  # OK (SSE2)
CFLAGS="-O1 -fno-inline" CC="gcc-12" make clean default  # OK (SSE2)

# gcc-11 -Og
CFLAGS="-Og"             CC="gcc-11" make clean default  # OK (SSE2)
Hi @jrosdahl, thanks for the info! Now I can reproduce this issue with the following command: ``` CFLAGS="-Og" CC="gcc-12" make clean default # NG ``` Since I have no idea so far, I just put some random commands for further investigation. ``` # gcc-12 -Og CFLAGS="-Og -no-mavx2" CC="gcc-12" make clean default # NG (SSE2) CFLAGS="-Og -msse2" CC="gcc-12" make clean default # NG (SSE2) CFLAGS="-Og -mavx2" CC="gcc-12" make clean default # OK (AVX2) CFLAGS="-Og -mhaswell" CC="gcc-12" make clean default # OK (AVX2) # gcc-12 -O? CFLAGS="-O0" CC="gcc-12" make clean default # OK (SSE2) CFLAGS="-O1" CC="gcc-12" make clean default # OK (SSE2) CFLAGS="-O1 -fno-inline" CC="gcc-12" make clean default # OK (SSE2) # gcc-11 -Og CFLAGS="-Og" CC="gcc-11" make clean default # OK (SSE2) ```
👍 2
tristan957 commented 2022-08-23 17:52:31 -05:00 (Migrated from github.com)

Just to add another data point, I can also confirm this. I have been investigating a little bit on my end.

Just to add another data point, I can also confirm this. I have been investigating a little bit on my end.
tristan957 commented 2022-08-23 19:02:03 -05:00 (Migrated from github.com)

I have fixed the meson build in https://github.com/mesonbuild/wrapdb/pull/580, specifically https://github.com/mesonbuild/wrapdb/pull/580/commits/e9b59a653f0b77a37aabe61d555f5a449649a455.

I think this PR has to do something similar, but I am not going to touch it because I have no clue how to tell what optimization level the compiler is using in Makefiles. I haven't found a pure C preprocessor solution. I would seriously just recommend using Meson for xxhash though :).

I have fixed the meson build in https://github.com/mesonbuild/wrapdb/pull/580, specifically https://github.com/mesonbuild/wrapdb/pull/580/commits/e9b59a653f0b77a37aabe61d555f5a449649a455. I think this PR has to do something similar, but I am not going to touch it because I have no clue how to tell what optimization level the compiler is using in Makefiles. I haven't found a pure C preprocessor solution. I would seriously just recommend using Meson for xxhash though :).
Cyan4973 commented 2023-02-02 16:55:13 -06:00 (Migrated from github.com)

Indeed, -Og seems to be "a compilation mode optimized for debugging".
Given the definition, I would expect this compilation mode to be rather hostile to inlining, since inlining can make debugging more difficult.

Somehow, -Og feels superficially similar to -O0 -g.

-O0 is detectable, by checking macro __NO_INLINE__ (cc @easyaspi314).
For some reason, -Og on gcc-12 doesn't expose the __NO_INLINE__ macro when selected.
If it's not going to respect the inline statements, it probably should.

At least, that's the situation with gcc-12. But clang on the other hand has no such problem.

I don't have access to older versions of gcc to check if the same pb happens.
Will try again later, when I can access another system with more compilers.

Indeed, `-Og` seems to be "a compilation mode optimized for debugging". Given the definition, I would expect this compilation mode to be rather hostile to inlining, since inlining can make debugging more difficult. Somehow, `-Og` feels superficially similar to `-O0 -g`. `-O0` is detectable, by checking macro `__NO_INLINE__` (cc @easyaspi314). For some reason, `-Og` on `gcc-12` doesn't expose the `__NO_INLINE__` macro when selected. If it's not going to respect the `inline` statements, it probably should. At least, that's the situation with `gcc-12`. But `clang` on the other hand has no such problem. I don't have access to older versions of `gcc` to check if the same pb happens. Will try again later, when I can access another system with more compilers.
easyaspi314 commented 2023-02-02 18:42:56 -06:00 (Migrated from github.com)

Hmm, this is a tricky one. GCC 12 doesn't seem to emit any different macros on -Og vs -O2.

github.com/gcc-mirror/gcc@c952126870 might be the culprit

The problem seems to be specifically -Og preventing GCC from inlining the accumulate functions because they are accessed through function pointers.

My guess is to either disable force inline on the accumulate functions themselves or force GCC into -O3 mode if neither -O0, -Os, or -fno-inline is used, similar to how GCC is forced to do in AVX2 (which by the way the load splitting bug seems to have been fixed, might need to find the commit to disable it on newer versions)

If we can get them to be inlined naturally (such as via static inline) this might work.

Hmm, this is a tricky one. GCC 12 doesn't seem to emit any different macros on `-Og` vs `-O2`. https://github.com/gcc-mirror/gcc/commit/c952126870c92cf293d59ffb1497e402eb8fc269 might be the culprit The problem seems to be specifically -Og preventing GCC from inlining the accumulate functions because they are accessed through function pointers. My guess is to either disable force inline on the accumulate functions themselves or force GCC into -O3 mode if neither `-O0`, `-Os`, or `-fno-inline` is used, similar to how GCC is forced to do in AVX2 (which by the way the load splitting bug seems to have been fixed, might need to find the commit to disable it on newer versions) If we can get them to be inlined naturally (such as via `static inline`) this might work.
👀 1
eugeneko commented 2023-05-28 07:47:07 -05:00 (Migrated from github.com)

I have also encountered this issue. Any workarounds except removing -Og from our compiler options?

I have also encountered this issue. Any workarounds except removing -Og from our compiler options?
tristan957 commented 2023-05-28 16:58:21 -05:00 (Migrated from github.com)

Using the Meson build will fix this issue if that is possible for you.

Using the Meson build will fix this issue if that is possible for you.
t-mat commented 2023-05-30 04:14:55 -05:00 (Migrated from github.com)

Hi @eugeneko, please try the HEAD of dev branch
or apply the following patch

Since #814 added CI test for gcc-12 -Og, we're sure that HEAD of dev branch has no problem with gcc-12 -Og.

You can also try it in your terminal

gcc-12 --version
# gcc-12 (Ubuntu 12.1.0-2ubuntu1~22.04) 12.1.0

cd
git clone https://github.com/Cyan4973/xxHash.git
cd xxHash

git branch -v
# * dev 616ca44 minor comment update

CFLAGS="-Og" CC="gcc-12" make clean default

./xxhsum --version
# xxhsum 0.8.1 by Yann Collet compiled as 64-bit x86_64 + SSE2 little endian with GCC 12.1.0
Hi @eugeneko, please try the HEAD of `dev` branch or apply the following patch - #804 Since #814 added CI test for `gcc-12 -Og`, we're sure that HEAD of `dev` branch has no problem with `gcc-12 -Og`. You can also try it in your terminal ``` gcc-12 --version # gcc-12 (Ubuntu 12.1.0-2ubuntu1~22.04) 12.1.0 cd git clone https://github.com/Cyan4973/xxHash.git cd xxHash git branch -v # * dev 616ca44 minor comment update CFLAGS="-Og" CC="gcc-12" make clean default ./xxhsum --version # xxhsum 0.8.1 by Yann Collet compiled as 64-bit x86_64 + SSE2 little endian with GCC 12.1.0 ```
👍 1
Cyan4973 commented 2023-06-24 14:45:47 -05:00 (Migrated from github.com)

Fixed in #804

Fixed in #804

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
announcement

   
bug

   
build

   
CI
Continuous Integration tests

  
dependencies
Pull requests that update a dependency file

  
documentation

   
duplicate

   
enhancement

   
experience feedback

   
feature request

   
github_actions
Pull requests that update GitHub Actions code

  
help wanted

   
install

   
invalid

   
Long-term

   
optimization

   
portability

   
question

   
target-specific
issue triggered in a specific environment (harware, compiler, OS)

  
test issue

   
wontfix
No labels
announcement
bug
build
CI
dependencies
documentation
duplicate
enhancement
experience feedback
feature request
github_actions
help wanted
install
invalid
Long-term
optimization
portability
question
target-specific
test issue
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-Cyan4973/xxHash!720
No description provided.