Page MenuHomeWildfire Games
Differential D3162 Diff 14312 source/graphics/Color.cpp

Changeset View

Standalone View

View Options

source/graphics/Color.cpp

/* Copyright (C) 2019 Wildfire Games. /* Copyright (C) 2020 Wildfire Games.
* This file is part of 0 A.D. * This file is part of 0 A.D.
* *
* 0 A.D. is free software: you can redistribute it and/or modify * 0 A.D. is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by * it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 2 of the License, or * the Free Software Foundation, either version 2 of the License, or
* (at your option) any later version. * (at your option) any later version.
* *
* 0 A.D. is distributed in the hope that it will be useful, * 0 A.D. is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of * but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details. * GNU General Public License for more details.
* *
* You should have received a copy of the GNU General Public License * You should have received a copy of the GNU General Public License
* along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. * along with 0 A.D. If not, see <http://www.gnu.org/licenses/>.
*/ */
#include "precompiled.h" #include "precompiled.h"
#include "graphics/Color.h" #include "graphics/Color.h"
#include "graphics/SColor.h" #include "graphics/SColor.h"
#include "maths/MathUtil.h" #include "maths/MathUtil.h"
#include "ps/CLogger.h" #include "ps/CLogger.h"
#include "ps/CStr.h" #include "ps/CStr.h"
#if HAVE_SSE #if HAVE_SSE
# include <xmmintrin.h> # include <xmmintrin.h>
#if ARCH_X86_X64
# include "lib/sysdep/arch/x86_x64/x86_x64.h" # include "lib/sysdep/arch/x86_x64/x86_x64.h"
#endif #endif
namespace
StanUnsubmitted
Not Done
Inline Actions

You should probably wrap this in the SSE macro above instead, else you'll get a warning about unused g_EnableSSE variable

Stan: You should probably wrap this in the SSE macro above instead, else you'll get a warning about…
{
#if ARCH_X86_X64
static bool g_EnableSSE = x86_x64::Cap(x86_x64::CAP_SSE);
StanUnsubmitted
Done
Inline Actions

inline aka return x86_x64::Cap(x86_x64::CAP_SSE);
No need for a static value

Stan: inline aka return x86_x64::Cap(x86_x64::CAP_SSE); No need for a static value
#elif ARCH_E2K
static bool g_EnableSSE = true;
#else
static bool g_EnableSSE = false;
#endif
vladislavbelovUnsubmitted
Not Done
Inline Actions

We don't have constexpr if so we can't initialize it dynamically. You might use a static function. Also you don't need to use HAVE_SSE here, since it's checked in-place.

vladislavbelov: We don't have `constexpr if` so we can't initialize it dynamically. You might use a static…
r-a-sattarovAuthorUnsubmitted
Done
Inline Actions

We don't have constexpr if so we can't initialize it dynamically. You might use a static function. Also you don't need to use HAVE_SSE here, since it's checked in-place.

I took source/renderer/ModelRenderer.cpp file as a sample.
I do an additional check for SSE because the use of SSE can be turned off in the MCST lcc compiler.

r-a-sattarov: > We don't have `constexpr if` so we can't initialize it dynamically. You might use a static…
StanUnsubmitted
Not Done
Inline Actions

Note that HAVE_SSE is overriden by our code. https://github.com/0ad/0ad/blob/f945c9b4c01e979d29a2317bf87795eeb3e34eb0/source/lib/sysdep/compiler.h#L100

Stan: Note that HAVE_SSE is overriden by our code. https://github.
StanUnsubmitted
Not Done
Inline Actions

Note that the check in ModelRender.cpp is defined inside a function :) here, you are not hence the failed builds :)

Stan: Note that the check in ModelRender.cpp is defined inside a function :) here, you are not hence…
r-a-sattarovAuthorUnsubmitted
Done
Inline Actions

Note that HAVE_SSE is overriden by our code. https://github.com/0ad/0ad/blob/f945c9b4c01e979d29a2317bf87795eeb3e34eb0/source/lib/sysdep/compiler.h#L100

Yes, but the macro "defined (_ _SSE_ _)" can be disabled in the MCST lcc compiler. Then we will have ARCH_E2K, but without SSE support, in which case there may be an error when trying to use Intel Intrinsics.

r-a-sattarov: > Note that HAVE_SSE is overriden by our code. https://github.
StanUnsubmitted
Not Done
Inline Actions

I mean that the macro you see here is not the one of your compiler, but ours, so your check should probably be in compiler.h :)

Stan: I mean that the macro you see here is not the one of your compiler, but ours, so your check…
r-a-sattarovAuthorUnsubmitted
Done
Inline Actions

I mean that the macro you see here is not the one of your compiler, but ours, so your check should probably be in compiler.h :)

The MCST lcc compiler is gcc compatible and can use this condition :)

# if GCC_VERSION && defined(__SSE__)
#  define HAVE_SSE 1
r-a-sattarov: > I mean that the macro you see here is not the one of your compiler, but ours, so your check…
StanUnsubmitted
Not Done
Inline Actions

Then you don't need the check here I suppose

Stan: Then you don't need the check here I suppose
r-a-sattarovAuthorUnsubmitted
Done
Inline Actions

Then you don't need the check here I suppose

I want to be sure that SSE are active in the MCST lcc compiler. Therefore, I make an additional check on this.
Like this

ARCH_E2K && HAVE_SSE

or like this

ARCH_E2K && defined(__SSE__)

I preferred the first option :)

In source/renderer/ModelRenderer.cpp file you make additional SSE check for x86-64 :)

#if HAVE_SSE
		if (g_EnableSSE)
r-a-sattarov: > Then you don't need the check here I suppose I want to be sure that SSE are active in the…
} // namespace
#endif // if HAVE_SSE
static SColor4ub fallback_ConvertRGBColorTo4ub(const RGBColor& src) static SColor4ub fallback_ConvertRGBColorTo4ub(const RGBColor& src)
StanUnsubmitted
Done
Inline Actions
#include "ps/CStr.h"
-
+ #if HAVE_SSE
#if ARCH_X86_X64
# include "lib/sysdep/arch/x86_x64/x86_x64.h"
#endif
-
- #if HAVE_SSE
# include <xmmintrin.h>
- #endif
-
namespace
{
#if ARCH_X86_X64
static bool g_EnableSSE = x86_x64::Cap(x86_x64::CAP_SSE);
- #elif ARCH_E2K && HAVE_SSE
+ #elif ARCH_E2K
static bool g_EnableSSE = true;
#else
static bool g_EnableSSE = false;
#endif
} // namespace
+ #endif
+
+
static SColor4ub fallback_ConvertRGBColorTo4ub(const RGBColor& src)

I mean this. :)

This way you:

  • don't include something for no reason when on ARCH_X86_X64
  • don't check HAVE_SSE too much
  • don't include g_EnableSSE when HAVE_SSE = 0
Stan: I mean this. :) This way you: - don't include something for no reason when on ARCH_X86_X64…
vladislavbelovUnsubmitted
Not Done
Inline Actions

It doesn't work like that (you see failed builds), you can't write a code in a global space (some strange compilers might allow though). I'd suggest to use a static function, like:

#if HAVE_SSE
#include <xmmintrin.h>

#if ARCH_X86_X64
# include "lib/sysdep/arch/x86_x64/x86_x64.h"
#endif

static bool IsSSEEnabled()
{
#if ARCH_X86_X64
	static const bool enabled = x86_x64::Cap(x86_x64::CAP_SSE);
	return enabled;
#elif ARCH_E2K
	return true;
#else
	return false;
#endif
}
#endif // HAVE_SSE

And it'd be good to reuse the function for ModelRenderer to reduce the logic duplication.

vladislavbelov: It doesn't work like that (you see failed builds), you can't write a code in a global space…
r-a-sattarovAuthorUnsubmitted
Done
Inline Actions

Thank you very much Vladislav! It looks like everything compiles without issue now :-)

r-a-sattarov: Thank you very much Vladislav! It looks like everything compiles without issue now :-)
{ {
SColor4ub result; SColor4ub result;
result.R = Clamp(static_cast<int>(src.X * 255), 0, 255); result.R = Clamp(static_cast<int>(src.X * 255), 0, 255);
result.G = Clamp(static_cast<int>(src.Y * 255), 0, 255); result.G = Clamp(static_cast<int>(src.Y * 255), 0, 255);
result.B = Clamp(static_cast<int>(src.Z * 255), 0, 255); result.B = Clamp(static_cast<int>(src.Z * 255), 0, 255);
result.A = 255; result.A = 255;
return result; return result;
} }
Show All 32 Linesstatic SColor4ub sse_ConvertRGBColorTo4ub(const RGBColor& src)
return SColor4ub(ri, gi, bi, 0xFF); return SColor4ub(ri, gi, bi, 0xFF);
} }
#endif #endif
void ColorActivateFastImpl() void ColorActivateFastImpl()
{ {
#if HAVE_SSE #if HAVE_SSE
if (x86_x64::Cap(x86_x64::CAP_SSE)) if (g_EnableSSE)
{ {
ConvertRGBColorTo4ub = sse_ConvertRGBColorTo4ub; ConvertRGBColorTo4ub = sse_ConvertRGBColorTo4ub;
return; return;
} }
#endif #endif
debug_printf("No SSE available. Slow fallback routines will be used.\n"); debug_printf("No SSE available. Slow fallback routines will be used.\n");
} }
▲ Show 20 Lines • Show All 56 Lines • Show Last 20 Lines
Wildfire Games · Phabricator