gh-Cyan4973/xxHash

Watch

Fork 0

Fix GCC 12 -Og #804

Merged

easyaspi314 merged 3 commits from gcc12-Og into dev

2023-03-09 16:59:31 -06:00

easyaspi314 commented

2023-02-27 16:59:16 -06:00

(Migrated from github.com)

The issue actually appears to be GCC disliking that a function pointer is SOMETIMES forceinline, and withSecret was marked force inline instead of noinline (despite what the comments suggested)

This marks XXH3_hashLong_{64,128}b_withSecret as XXH_NO_INLINE instead of XXH_FORCE_INLINE like the rest of the hashLong functions.

The issue actually appears to be GCC disliking that a function pointer is SOMETIMES forceinline, and withSecret was marked force inline instead of noinline (despite what the comments suggested) This marks `XXH3_hashLong_{64,128}b_withSecret` as `XXH_NO_INLINE` instead of `XXH_FORCE_INLINE` like the rest of the `hashLong` functions.

👍 1 ❤️ 1

easyaspi314 commented

2023-02-27 17:01:16 -06:00

(Migrated from github.com)

It might be a good idea to add -Og and GCC 12 to CI.

It might be a good idea to add `-Og` and GCC 12 to CI.

👍 2

easyaspi314 commented

2023-02-27 17:02:31 -06:00

(Migrated from github.com)

Fixes #800 #720
improved version of fix in #802

Fixes #800 #720 improved version of fix in #802

Cyan4973 commented

2023-02-27 17:20:52 -06:00

(Migrated from github.com)

It might be a good idea to add -Og and GCC 12 to CI.

Agreed

> It might be a good idea to add `-Og` and GCC 12 to CI. Agreed

easyaspi314 commented

2023-02-27 17:34:56 -06:00

(Migrated from github.com)

I misread the comment. Perhaps this?

#if __GNUC__ >= 12 || !defined(XXH_INLINE_ALL)
XXH_NO_INLINE
#else
XXH_FORCE_INLINE
#endif

I misread the comment. Perhaps this? ```c #if __GNUC__ >= 12 || !defined(XXH_INLINE_ALL) XXH_NO_INLINE #else XXH_FORCE_INLINE #endif ```

easyaspi314 commented

2023-02-27 18:19:29 -06:00

(Migrated from github.com)

I now made it force inline if and only if XXH_INLINE_ALL is defined and it isn't GCC 12

Cyan4973 (Migrated from github.com) reviewed

2023-02-27 18:37:23 -06:00

xxhash.h

This should probably be abstracted behind a dedicated macro, that can then be shared by the 2 places where it's invoked.

Cyan4973 (Migrated from github.com) reviewed

2023-02-27 18:46:51 -06:00

xxhash.h

The comment explains that, when secret's size is static,
then it's advantageous for performance to pass that to the function.
This suggests that the constant size is propagated inside XXH3_hashLong_64b_withSecret() for further optimization.

Of course, if we use XXH_NO_INLINE, this property is lost.

Now, this could be nonetheless an acceptable trade-off if the goal is to survive the specific gcc-12 -Og compilation scenario.

But my understanding of below macro test is that it's more general :

It's applicable to any current and future version of gcc, >= 12
XXH_NO_INLINE is always set (not just when -Og is set), with the exception when inlining is expressly required with XXH_INLINE_ALL.

So, effectively, this will impact performance on a broader range of scenarios.

And I wonder if this is the right trade off.

The comment explains that, when secret's size is `static`, then it's advantageous for performance to pass that to the function. This suggests that the constant size is propagated inside `XXH3_hashLong_64b_withSecret()` for further optimization. Of course, if we use `XXH_NO_INLINE`, this property is lost. Now, this could be nonetheless an acceptable trade-off if the goal is to survive the specific `gcc-12 -Og` compilation scenario. But my understanding of below macro test is that it's more general : 1) It's applicable to any current and future version of `gcc`, >= 12 2) `XXH_NO_INLINE` is _always_ set (not just when `-Og` is set), with the exception when inlining is expressly required with `XXH_INLINE_ALL`. So, effectively, this will impact performance on a broader range of scenarios. And I wonder if this is the right trade off.

easyaspi314 (Migrated from github.com) reviewed

2023-02-27 20:44:35 -06:00

xxhash.h

When XXH3 is not inlined, there isn't much of a benefit to inlining XXH3_hashLong_withSecret, as the secret size would be entirely unknown.

Cyan4973 (Migrated from github.com) reviewed

2023-02-27 23:34:45 -06:00

xxhash.h

That's a good point.
For some reason, I expected this variant to be within the call chain of the "default" variant using the default kSecret,
but it's not, "default" delegates directly to XXH3_hashLong_64b_internal().

That's a good point. For some reason, I expected this variant to be within the call chain of the "default" variant using the default `kSecret`, but it's not, "default" delegates directly to `XXH3_hashLong_64b_internal()`.

Cyan4973 (Migrated from github.com) approved these changes

2023-03-01 16:17:42 -06:00

Cyan4973 commented

2023-03-09 11:19:50 -06:00

(Migrated from github.com)

I'm a little lost about this gcc-12 -Og issue.
It seems that #814 fixes it, using a different initialization strategy.

This PR seems to address the same issue, but in a very different way (inline).
Is it still useful despite #814 ?

I'm a little lost about this `gcc-12 -Og` issue. It seems that #814 fixes it, using a different initialization strategy. This PR seems to address the same issue, but in a very different way (`inline`). Is it still useful despite #814 ?

easyaspi314 commented

2023-03-09 16:57:16 -06:00

(Migrated from github.com)

This specific issue comes from the "failed to inline" errors, the other issue is unrelated.

Basically, the issue is that GCC 12 on -Og does not like a function pointer that can either point to something marked always_inline or noinline, and it errors.

This specific issue comes from the "failed to inline" errors, the other issue is unrelated. Basically, the issue is that GCC 12 on -Og does not like a function pointer that can either point to something marked `always_inline` or `noinline`, and it errors.

👍 1

tristan957 commented

2023-06-24 16:42:56 -05:00

(Migrated from github.com)