From 2e293ee7cbc6f0dbd069ba7c678d20c1af16872f Mon Sep 17 00:00:00 2001 From: Chris Robinson Date: Sat, 16 Sep 2023 06:19:02 -0700 Subject: Don't inline some big functions This is very dumb. Template functions are implicitly marked inline according to C++, and contrary to popular belief, "inline" *does* influence the compiler's decision to inline a function. A function the compiler may not have decided to inline normally may be inlined anyway, or issue a warning if it still decides not to inline, if explicitly or implicitly marked as such (or does inline it as requested, but then decides to not inline different functions it normally would because of a cumulative code size increase or something). Furthermore, once a function becomes inline due to being a template function, there's no way to undo it. Marking an inline function "noinline" pushes the problem the other way, causing the compiler to not inline a function it may have decided was beneficial to inline. There's no way to declare a template function to be inlined based solely on compiler heuristics, it will always be influenced by the implicit "inline" or explicit "noinline". That's what's happening here. A number of functions had been combined into a smaller number of large-ish template functions to reduce code duplication and ease maintanence, causing them to be implicitly inline as a side-effect. GCC then manages to inline these larger functions as implicitly requested, but in doing so prevents other smaller functions (which are explicitly marked inline) from being inlined due to excessive code increase and issue a warning. The "noinline" is a heavy-handed method of un-pessimizing the optimization pass, on the assumption the compiler apparently doesn't actually want to inline the template functions, but does so because they're technically marked inline. There's no good option here until it gets acknowledged that inline does mean something beyond allowing multiple definitions, and that template (and other types of) function definitions sometimes (if not most often) want to allow multiple definitions but don't want an artificial/detrimental boost in inline prioritization. /rant --- al/source.cpp | 72 ++++++++++++++++++++++++----------------------------- common/opthelpers.h | 3 +++ 2 files changed, 35 insertions(+), 40 deletions(-) diff --git a/al/source.cpp b/al/source.cpp index 7aa8fbe7..d5ac8f45 100644 --- a/al/source.cpp +++ b/al/source.cpp @@ -1422,9 +1422,9 @@ auto GetCheckers(ALCcontext *const Context, const SourceProp prop, const al::spa ); } -template -void SetProperty(ALsource *const Source, ALCcontext *const Context, const SourceProp prop, - const al::span values) try +template +noinline void SetProperty(ALsource *const Source, ALCcontext *const Context, const SourceProp prop, + const al::span values) try { auto&& [CheckSize, CheckValue] = GetCheckers(Context, prop, values); ALCdevice *device{Context->mALDevice.get()}; @@ -2007,10 +2007,10 @@ auto GetSizeChecker(ALCcontext *const Context, const SourceProp prop, const al:: }; } -template -[[nodiscard]] +template +[[nodiscard]] noinline bool GetProperty(ALsource *const Source, ALCcontext *const Context, const SourceProp prop, - const al::span values) try + const al::span values) try { using std::chrono::duration_cast; auto CheckSize = GetSizeChecker(Context, prop, values); @@ -2726,8 +2726,7 @@ FORCE_ALIGN void AL_APIENTRY alSourcefDirect(ALCcontext *context, ALuint source, if(!Source) UNLIKELY return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); - SetProperty(Source, context, static_cast(param), - al::span{&value, 1u}); + SetProperty(Source, context, static_cast(param), al::span{&value, 1u}); } AL_API DECL_FUNC5(void, alSource3f, ALuint, ALenum, ALfloat, ALfloat, ALfloat) @@ -2741,7 +2740,7 @@ FORCE_ALIGN void AL_APIENTRY alSource3fDirect(ALCcontext *context, ALuint source return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); const float fvals[3]{ value1, value2, value3 }; - SetProperty(Source, context, static_cast(param), al::span{fvals}); + SetProperty(Source, context, static_cast(param), al::span{fvals}); } AL_API DECL_FUNC3(void, alSourcefv, ALuint, ALenum, const ALfloat*) @@ -2771,8 +2770,7 @@ FORCE_ALIGN void AL_APIENTRY alSourcedDirectSOFT(ALCcontext *context, ALuint sou if(!Source) UNLIKELY return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); - SetProperty(Source, context, static_cast(param), - al::span{&value, 1}); + SetProperty(Source, context, static_cast(param), al::span{&value, 1}); } AL_API DECL_FUNCEXT5(void, alSource3d,SOFT, ALuint, ALenum, ALdouble, ALdouble, ALdouble) @@ -2786,7 +2784,7 @@ FORCE_ALIGN void AL_APIENTRY alSource3dDirectSOFT(ALCcontext *context, ALuint so return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); const double dvals[3]{value1, value2, value3}; - SetProperty(Source, context, static_cast(param), al::span{dvals}); + SetProperty(Source, context, static_cast(param), al::span{dvals}); } AL_API DECL_FUNCEXT3(void, alSourcedv,SOFT, ALuint, ALenum, const ALdouble*) @@ -2816,8 +2814,7 @@ FORCE_ALIGN void AL_APIENTRY alSourceiDirect(ALCcontext *context, ALuint source, if(!Source) UNLIKELY return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); - SetProperty(Source, context, static_cast(param), - al::span{&value, 1u}); + SetProperty(Source, context, static_cast(param), al::span{&value, 1u}); } AL_API DECL_FUNC5(void, alSource3i, ALuint, ALenum, ALint, ALint, ALint) @@ -2831,7 +2828,7 @@ FORCE_ALIGN void AL_APIENTRY alSource3iDirect(ALCcontext *context, ALuint source return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); const int ivals[3]{ value1, value2, value3 }; - SetProperty(Source, context, static_cast(param), al::span{ivals}); + SetProperty(Source, context, static_cast(param), al::span{ivals}); } AL_API DECL_FUNC3(void, alSourceiv, ALuint, ALenum, const ALint*) @@ -2861,8 +2858,7 @@ FORCE_ALIGN void AL_APIENTRY alSourcei64DirectSOFT(ALCcontext *context, ALuint s if(!Source) UNLIKELY return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); - SetProperty(Source, context, static_cast(param), - al::span{&value, 1u}); + SetProperty(Source, context, static_cast(param), al::span{&value, 1u}); } AL_API DECL_FUNCEXT5(void, alSource3i64,SOFT, ALuint, ALenum, ALint64SOFT, ALint64SOFT, ALint64SOFT) @@ -2876,7 +2872,7 @@ FORCE_ALIGN void AL_APIENTRY alSource3i64DirectSOFT(ALCcontext *context, ALuint return context->setError(AL_INVALID_NAME, "Invalid source ID %u", source); const int64_t i64vals[3]{ value1, value2, value3 }; - SetProperty(Source, context, static_cast(param), al::span{i64vals}); + SetProperty(Source, context, static_cast(param), al::span{i64vals}); } AL_API DECL_FUNCEXT3(void, alSourcei64v,SOFT, ALuint, ALenum, const ALint64SOFT*) @@ -2907,8 +2903,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSourcefDirect(ALCcontext *context, ALuint sour if(!value) UNLIKELY return context->setError(AL_INVALID_VALUE, "NULL pointer"); - std::ignore = GetProperty(Source, context, static_cast(param), - al::span{value, 1}); + std::ignore = GetProperty(Source, context, static_cast(param), al::span{value, 1}); } AL_API DECL_FUNC5(void, alGetSource3f, ALuint, ALenum, ALfloat*, ALfloat*, ALfloat*) @@ -2923,7 +2918,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSource3fDirect(ALCcontext *context, ALuint sou return context->setError(AL_INVALID_VALUE, "NULL pointer"); float fvals[3]; - if(GetProperty(Source, context, static_cast(param), al::span{fvals})) + if(GetProperty(Source, context, static_cast(param), al::span{fvals})) { *value1 = fvals[0]; *value2 = fvals[1]; @@ -2959,8 +2954,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSourcedDirectSOFT(ALCcontext *context, ALuint if(!value) UNLIKELY return context->setError(AL_INVALID_VALUE, "NULL pointer"); - std::ignore = GetProperty(Source, context, static_cast(param), - al::span{value, 1u}); + std::ignore = GetProperty(Source, context, static_cast(param), al::span{value, 1}); } AL_API DECL_FUNCEXT5(void, alGetSource3d,SOFT, ALuint, ALenum, ALdouble*, ALdouble*, ALdouble*) @@ -2975,7 +2969,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSource3dDirectSOFT(ALCcontext *context, ALuint return context->setError(AL_INVALID_VALUE, "NULL pointer"); double dvals[3]; - if(GetProperty(Source, context, static_cast(param), al::span{dvals})) + if(GetProperty(Source, context, static_cast(param), al::span{dvals})) { *value1 = dvals[0]; *value2 = dvals[1]; @@ -3011,8 +3005,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSourceiDirect(ALCcontext *context, ALuint sour if(!value) UNLIKELY return context->setError(AL_INVALID_VALUE, "NULL pointer"); - std::ignore = GetProperty(Source, context, static_cast(param), - al::span{value, 1u}); + std::ignore = GetProperty(Source, context, static_cast(param), al::span{value, 1}); } AL_API DECL_FUNC5(void, alGetSource3i, ALuint, ALenum, ALint*, ALint*, ALint*) @@ -3027,7 +3020,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSource3iDirect(ALCcontext *context, ALuint sou return context->setError(AL_INVALID_VALUE, "NULL pointer"); int ivals[3]; - if(GetProperty(Source, context, static_cast(param), al::span{ivals})) + if(GetProperty(Source, context, static_cast(param), al::span{ivals})) { *value1 = ivals[0]; *value2 = ivals[1]; @@ -3062,8 +3055,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSourcei64DirectSOFT(ALCcontext *context, ALuin if(!value) UNLIKELY return context->setError(AL_INVALID_VALUE, "NULL pointer"); - std::ignore = GetProperty(Source, context, static_cast(param), - al::span{value, 1u}); + std::ignore = GetProperty(Source, context, static_cast(param), al::span{value, 1}); } AL_API DECL_FUNCEXT5(void, alGetSource3i64,SOFT, ALuint, ALenum, ALint64SOFT*, ALint64SOFT*, ALint64SOFT*) @@ -3078,7 +3070,7 @@ FORCE_ALIGN void AL_APIENTRY alGetSource3i64DirectSOFT(ALCcontext *context, ALui return context->setError(AL_INVALID_VALUE, "NULL pointer"); int64_t i64vals[3]; - if(GetProperty(Source, context, static_cast(param), al::span{i64vals})) + if(GetProperty(Source, context, static_cast(param), al::span{i64vals})) { *value1 = i64vals[0]; *value2 = i64vals[1]; @@ -3141,11 +3133,11 @@ FORCE_ALIGN void AL_APIENTRY alSourcePlayvDirect(ALCcontext *context, ALsizei n, std::array source_storage; al::span srchandles; if(static_cast(n) <= source_storage.size()) LIKELY - srchandles = {source_storage.data(), static_cast(n)}; + srchandles = al::span{source_storage}.subspan(static_cast(n)); else { extra_sources.resize(static_cast(n)); - srchandles = {extra_sources.data(), extra_sources.size()}; + srchandles = extra_sources; } std::lock_guard _{context->mSourceLock}; @@ -3175,11 +3167,11 @@ FORCE_ALIGN void AL_APIENTRY alSourcePlayAtTimevDirectSOFT(ALCcontext *context, std::array source_storage; al::span srchandles; if(static_cast(n) <= source_storage.size()) LIKELY - srchandles = {source_storage.data(), static_cast(n)}; + srchandles = al::span{source_storage}.subspan(static_cast(n)); else { extra_sources.resize(static_cast(n)); - srchandles = {extra_sources.data(), extra_sources.size()}; + srchandles = extra_sources; } std::lock_guard _{context->mSourceLock}; @@ -3211,11 +3203,11 @@ FORCE_ALIGN void AL_APIENTRY alSourcePausevDirect(ALCcontext *context, ALsizei n std::array source_storage; al::span srchandles; if(static_cast(n) <= source_storage.size()) LIKELY - srchandles = {source_storage.data(), static_cast(n)}; + srchandles = al::span{source_storage}.subspan(static_cast(n)); else { extra_sources.resize(static_cast(n)); - srchandles = {extra_sources.data(), extra_sources.size()}; + srchandles = extra_sources; } std::lock_guard _{context->mSourceLock}; @@ -3283,11 +3275,11 @@ FORCE_ALIGN void AL_APIENTRY alSourceStopvDirect(ALCcontext *context, ALsizei n, std::array source_storage; al::span srchandles; if(static_cast(n) <= source_storage.size()) LIKELY - srchandles = {source_storage.data(), static_cast(n)}; + srchandles = al::span{source_storage}.subspan(static_cast(n)); else { extra_sources.resize(static_cast(n)); - srchandles = {extra_sources.data(), extra_sources.size()}; + srchandles = extra_sources; } std::lock_guard _{context->mSourceLock}; @@ -3342,11 +3334,11 @@ FORCE_ALIGN void AL_APIENTRY alSourceRewindvDirect(ALCcontext *context, ALsizei std::array source_storage; al::span srchandles; if(static_cast(n) <= source_storage.size()) LIKELY - srchandles = {source_storage.data(), static_cast(n)}; + srchandles = al::span{source_storage}.subspan(static_cast(n)); else { extra_sources.resize(static_cast(n)); - srchandles = {extra_sources.data(), extra_sources.size()}; + srchandles = extra_sources; } std::lock_guard _{context->mSourceLock}; diff --git a/common/opthelpers.h b/common/opthelpers.h index 596c2455..cc606b9e 100644 --- a/common/opthelpers.h +++ b/common/opthelpers.h @@ -19,10 +19,13 @@ #ifdef __GNUC__ #define force_inline [[gnu::always_inline]] inline +#define noinline [[gnu::noinline]] #elif defined(_MSC_VER) #define force_inline __forceinline +#define noinline __declspec(noinline) #else #define force_inline inline +#define noinline #endif /* Unlike the likely attribute, ASSUME requires the condition to be true or -- cgit v1.2.3