From b79375a0c54a910b58e2f0b09017ebf5429064ce Mon Sep 17 00:00:00 2001 From: janwas Date: Mon, 30 Jun 2008 19:08:29 +0000 Subject: [PATCH] color conversion: avoid type punning by returning SColor4ub directly. that required a non-extern-C declaration of the SSE function, so i replaced it with straightforward intrinsics. ia32: no longer needs #if since build system ensures it's not compiled on amd64 This was SVN commit r6164. --- source/graphics/Color.cpp | 45 +++++++++++++++++++++---- source/graphics/Color.h | 3 +- source/graphics/Color_asm.asm | 50 ---------------------------- source/graphics/tests/test_Color.h | 22 ++++++++---- source/lib/sysdep/arch/ia32/ia32.cpp | 4 --- source/renderer/ModelRenderer.cpp | 4 +-- source/renderer/PatchRData.cpp | 2 +- 7 files changed, 58 insertions(+), 72 deletions(-) delete mode 100644 source/graphics/Color_asm.asm diff --git a/source/graphics/Color.cpp b/source/graphics/Color.cpp index 6ac6334659..e6edae6955 100644 --- a/source/graphics/Color.cpp +++ b/source/graphics/Color.cpp @@ -12,25 +12,56 @@ #include "maths/MathUtil.h" #include "graphics/SColor.h" -#include "lib/sysdep/arch/x86_x64/x86_x64.h" -static u32 fallback_ConvertRGBColorTo4ub(const RGBColor& src) +#if ARCH_IA32 || ARCH_AMD64 +# include +# include "lib/sysdep/arch/x86_x64/x86_x64.h" +#endif + +static SColor4ub fallback_ConvertRGBColorTo4ub(const RGBColor& src) { SColor4ub result; result.R=clamp(int(src.X*255),0,255); result.G=clamp(int(src.Y*255),0,255); result.B=clamp(int(src.Z*255),0,255); result.A=0xff; - return *(u32*)&result; + return result; } // on IA32, this is replaced by an SSE assembly version in ia32.cpp -u32 (*ConvertRGBColorTo4ub)(const RGBColor& src) = fallback_ConvertRGBColorTo4ub; +SColor4ub (*ConvertRGBColorTo4ub)(const RGBColor& src) = fallback_ConvertRGBColorTo4ub; // Assembler-optimized function for color conversion -#if ARCH_IA32 -EXTERN_C u32 sse_ConvertRGBColorTo4ub(const RGBColor& src); +#if ARCH_IA32 || ARCH_AMD64 +static SColor4ub sse_ConvertRGBColorTo4ub(const RGBColor& src) +{ + const __m128 zero = _mm_setzero_ps(); + const __m128 _255 = _mm_set_ss(255.0f); + __m128 r = _mm_load_ss(&src.X); + __m128 g = _mm_load_ss(&src.Y); + __m128 b = _mm_load_ss(&src.Z); + + // C = min(255, 255*max(C, 0)) ( == clamp(255*C, 0, 255) ) + r = _mm_max_ss(r, zero); + g = _mm_max_ss(g, zero); + b = _mm_max_ss(b, zero); + + r = _mm_mul_ss(r, _255); + g = _mm_mul_ss(g, _255); + b = _mm_mul_ss(b, _255); + + r = _mm_min_ss(r, _255); + g = _mm_min_ss(g, _255); + b = _mm_min_ss(b, _255); + + // convert to integer and combine channels using bit logic + int ri = _mm_cvtss_si32(r); + int gi = _mm_cvtss_si32(g); + int bi = _mm_cvtss_si32(b); + + return SColor4ub(ri, gi, bi, 0xFF); +} #endif void ColorActivateFastImpl() @@ -38,7 +69,7 @@ void ColorActivateFastImpl() if(0) { } -#if ARCH_IA32 +#if ARCH_IA32 || ARCH_AMD64 else if (x86_x64_cap(X86_X64_CAP_SSE)) { ConvertRGBColorTo4ub = sse_ConvertRGBColorTo4ub; diff --git a/source/graphics/Color.h b/source/graphics/Color.h index b9b76fe45b..0a59b21b01 100644 --- a/source/graphics/Color.h +++ b/source/graphics/Color.h @@ -9,6 +9,7 @@ #ifndef INCLUDED_COLOR #define INCLUDED_COLOR +#include "SColor.h" #include "maths/Vector3D.h" #include "maths/Vector4D.h" @@ -19,7 +20,7 @@ typedef CVector4D RGBAColor; // exposed as function pointer because it is set at init-time to // one of several implementations depending on CPU caps. -extern u32 (*ConvertRGBColorTo4ub)(const RGBColor& src); +extern SColor4ub (*ConvertRGBColorTo4ub)(const RGBColor& src); // call once ia32_Init has run; detects CPU caps and activates the best // possible codepath. diff --git a/source/graphics/Color_asm.asm b/source/graphics/Color_asm.asm deleted file mode 100644 index 754ef06c48..0000000000 --- a/source/graphics/Color_asm.asm +++ /dev/null @@ -1,50 +0,0 @@ -%include "../lib/sysdep/arch/ia32/ia32.inc" - -;------------------------------------------------------------------------------- -; Color conversion (SSE) -;------------------------------------------------------------------------------- - -; extern "C" u32 ConvertRGBColorTo4ub(const RGBColor& color) -[section .data] - align 16 -zero: - dd 0.0 -twofivefive: - dd 255.0 - - -__SECT__ - align 16 -global sym(sse_ConvertRGBColorTo4ub) -sym(sse_ConvertRGBColorTo4ub): - mov eax, [esp+4] - - ; xmm0, 1, 2 = R, G, B - movss xmm4, [zero] - movss xmm0, [eax+8] - movss xmm1, [eax+4] - movss xmm2, [eax] - movss xmm5, [twofivefive] - - ; C = min(255, 255*max(C, 0)) ( == clamp(255*C, 0, 255) ) - maxss xmm0, xmm4 - maxss xmm1, xmm4 - maxss xmm2, xmm4 - mulss xmm0, xmm5 - mulss xmm1, xmm5 - mulss xmm2, xmm5 - minss xmm0, xmm5 - minss xmm1, xmm5 - minss xmm2, xmm5 - - ; convert to integer and combine channels using bit logic - cvtss2si eax, xmm0 - cvtss2si ecx, xmm1 - cvtss2si edx, xmm2 - shl eax, 16 - shl ecx, 8 - or eax, 0xff000000 - or edx, ecx - or eax, edx - - ret diff --git a/source/graphics/tests/test_Color.h b/source/graphics/tests/test_Color.h index 860ae7ae56..8472ebc5d5 100644 --- a/source/graphics/tests/test_Color.h +++ b/source/graphics/tests/test_Color.h @@ -12,12 +12,20 @@ public: void test_Color4ub() { -#define T(r, g, b, ub) TS_ASSERT_EQUALS(ub | 0xff000000, ConvertRGBColorTo4ub(RGBColor(r,g,b))) - T(0, 0, 0, 0x000000); - T(1, 0, 0, 0x0000ff); - T(0, 1, 0, 0x00ff00); - T(0, 0, 1, 0xff0000); - T(1, 1, 1, 0xffffff); -#undef T + CheckColor(0, 0, 0, 0x000000); + CheckColor(1, 0, 0, 0x0000ff); + CheckColor(0, 1, 0, 0x00ff00); + CheckColor(0, 0, 1, 0xff0000); + CheckColor(1, 1, 1, 0xffffff); + } + +private: + void CheckColor(int r, int g, int b, u32 expected) + { + SColor4ub colorStruct = ConvertRGBColorTo4ub(RGBColor(r,g,b)); + u32 actual; + memcpy(&actual, &colorStruct, sizeof(u32)); + expected |= 0xff000000; // ConvertRGBColorTo4ub sets alpha to opaque + TS_ASSERT_EQUALS(expected, actual); } }; diff --git a/source/lib/sysdep/arch/ia32/ia32.cpp b/source/lib/sysdep/arch/ia32/ia32.cpp index 4a0b097055..d849410f75 100644 --- a/source/lib/sysdep/arch/ia32/ia32.cpp +++ b/source/lib/sysdep/arch/ia32/ia32.cpp @@ -10,8 +10,6 @@ #include "precompiled.h" -#if ARCH_IA32 - #include "ia32.h" #include "lib/sysdep/cpu.h" @@ -114,5 +112,3 @@ void* cpu_memcpy(void* RESTRICT dst, const void* RESTRICT src, size_t size) { return ia32_memcpy(dst, src, size); } - -#endif // ARCH_IA32 diff --git a/source/renderer/ModelRenderer.cpp b/source/renderer/ModelRenderer.cpp index 920cb414ce..d45d1f4442 100644 --- a/source/renderer/ModelRenderer.cpp +++ b/source/renderer/ModelRenderer.cpp @@ -116,7 +116,7 @@ void ModelRenderer::BuildColor4ub( tempcolor.X *= shadingColor.r; tempcolor.Y *= shadingColor.g; tempcolor.Z *= shadingColor.b; - *(u32*)&Color[j] = ConvertRGBColorTo4ub(tempcolor); + Color[j] = ConvertRGBColorTo4ub(tempcolor); } } else @@ -127,7 +127,7 @@ void ModelRenderer::BuildColor4ub( tempcolor.X *= shadingColor.r; tempcolor.Y *= shadingColor.g; tempcolor.Z *= shadingColor.b; - *(u32*)&Color[j] = ConvertRGBColorTo4ub(tempcolor); + Color[j] = ConvertRGBColorTo4ub(tempcolor); } } } diff --git a/source/renderer/PatchRData.cpp b/source/renderer/PatchRData.cpp index 830e27a6f8..35be2d9d4e 100644 --- a/source/renderer/PatchRData.cpp +++ b/source/renderer/PatchRData.cpp @@ -376,7 +376,7 @@ void CPatchRData::BuildVertices() RGBColor diffuse; lightEnv.EvaluateDirect(normal, diffuse); - *(u32*)&vertices[v].m_DiffuseColor = ConvertRGBColorTo4ub(diffuse); + vertices[v].m_DiffuseColor = ConvertRGBColorTo4ub(diffuse); } }