CI: Add GitHub Actions

[Biswa96]

No description provided.

[bkaradzic]

I noticed MinGW64 started failing: https://github.com/bkaradzic/bgfx/actions/runs/3795872643/jobs/6488548146

It seems like something changed with MinGW container, since code didn't change for a while... Any ideas what might be going on?

[Biswa96]

Probably SSE has to be enabled explicitly. Let me check it.

[bkaradzic]

Sure, but I wouldn't expect that CI breaks on unrelated change. Unless container is not fixed to specific version.

[Biswa96]

astc-encoder has some SSE flags in CMakeLists.txt. So, this sounds like expected behavior. Though I agree on this sudden change.

[bkaradzic]

Also this is x64 build so _mm_shuffle_epi8 is SSSE3 intrinsic and it should be available without any additional options.

https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm_shuffle_epi8&ig_expand=6559

[Biswa96]

The following change workaround the compiler error:

--- a/scripts/bimg.lua
+++ b/scripts/bimg.lua
@@ -36,4 +36,9 @@ project "bimg"
 			"-fPIC",
 		}
 
+	configuration { "mingw-*" }
+		buildoptions {
+			"-msse4.1",
+		}
+
 	configuration {}

Please let me ask others about this topic. I can not figure out the reason for this.

[Biswa96]

OK, found the answer.

SSSE3 intrinsic and it should be available without any additional options.

No, says Microsoft docs.

To obey the hardware requirement rules, the global compiler flag in msys2/mingw environment was set to -march=nocona -msahf. It does not enable SSSE3. That flag was added recently in gcc. Hence this issue arises.

This issue would not happen in other mingw environment. So, you can add -mssse3 in bimg.lua file as above. Or just add it in CI yaml file. Which one do you prefer? I can add a pull request with that change.

[bkaradzic]

Can we make this work with some fixed version of MinGW so that this type error on CI doesn't happen?

I don't understand why they went from generic x86-64 to some ancient nocnona arch?!
https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html

[Biswa96]

Can we make this work with some fixed version of MinGW so that this type error on CI doesn't happen?

It will not change in upcoming 5 or 10 years.

I don't understand why they went from generic x86-64 to some ancient nocnona arch?!

To satisfy Windows hardware requirements condition, here are the list for Windows 10

• Compatible with the x86* or x64 instruction set.
• Supports PAE, NX and SSE2.
• Supports CMPXCHG16b, LAHF/SAHF, and PrefetchW for 64-bit OS installation

Also the -march conditions have to satisfy 32 bit x86 machines. Hence, nocona.

[bkaradzic]

I'm checking Steam HW Survey, and I'll bump all builds to SSE4.2, 98.98% users have it:
https://store.steampowered.com/hwsurvey

[Biswa96]

I'll bump all builds to SSE4.2, 98.98% users have it

Windows 11 requires support for PAE, NX and SSE4.1. I am not sure if enabling SSE4.2 would exclude some of the users. It's OK for me though.

[bkaradzic]

After updating to SSE4.2 I found that MinGW version of popcntintrin.h has some nonsense x64 ifdef for _mm_popcnt_u64 so I had to fix this too:

https://github.com/bkaradzic/bimg/blob/1395f4e969fa0aac6158fb3caf0873eaed38d77f/3rdparty/astc-encoder/source/astcenc_vecmathlib_sse_4.h#L1276-L1283

[bkaradzic]

Fixed! Thanks for help!

[Biswa96]

MinGW version of popcntintrin.h has some nonsense x64 ifdef for _mm_popcnt_u64

Can you provide the compiler error message for that issue? I can try to ask mingw-w64 developers to add that.

[bkaradzic]

It's this:
https://github.com/gcc-mirror/gcc/blob/ec1db9017939bb8289c9bd63aace66c0f3957ecd/gcc/config/i386/popcntintrin.h#L40-L46

When targeting 32-bit, it says _mm_popcnt_u64 doesn't exist.

[VVD]

I got this build error related to popcnt and i386:

/wrkdirs/usr/ports/graphics/bgfx/work/bgfx.cmake-1.127.8725-465/bimg/3rdparty/astc-encoder/source/astcenc_vecmathlib_sse_4.h:1313:26: error: use of undeclared identifier '_mm_popcnt_u64'; did you mean '_mm_popcnt_u32'?
        return static_cast<int>(_mm_popcnt_u64(v));
                                ^~~~~~~~~~~~~~
                                _mm_popcnt_u32
/usr/lib/clang/14.0.5/include/popcntintrin.h:33:1: note: '_mm_popcnt_u32' declared here
_mm_popcnt_u32(unsigned int __A)
^
1 error generated.

FreeBSD 13.2 i386, llvm 14.0.5, -march=nehalem or newer, as a workaround I can use -march=penryn to prevent usage of popcnt.

This patch work for me:

--- bimg/3rdparty/astc-encoder/source/astcenc_vecmathlib_sse_4.h.orig
+++ bimg/3rdparty/astc-encoder/source/astcenc_vecmathlib_sse_4.h
@@ -1309,5 +1309,7 @@ ASTCENC_SIMD_INLINE int popcount(uint64_t v)
 {
 #if defined(__MINGW32__)
        return static_cast<int>(__builtin_popcountll(v));
+#elif defined(__FreeBSD__) && !defined(__x86_64__)
+       return static_cast<int>(_mm_popcnt_u32(static_cast<uint32_t>(v)));
 #else
        return static_cast<int>(_mm_popcnt_u64(v));
 #endif // defined(__MINGW32__)

P.S. If necessary, I can create a separate issue.

[bkaradzic]

P.S. If necessary, I can create a separate issue.

@VVD No need for separate issue FreeBSD is not supported platform:
https://github.com/bkaradzic/bx/blob/b95012f14bc6c4e4d0a052f637d178c1a8d707d9/include/bx/platform.h#L487-L499

[VVD]

P.S. If necessary, I can create a separate issue.

@VVD No need for separate issue FreeBSD is not supported platform: https://github.com/bkaradzic/bx/blob/b95012f14bc6c4e4d0a052f637d178c1a8d707d9/include/bx/platform.h#L487-L499

@bkaradzic, we have port with custom patches: https://cgit.freebsd.org/ports/tree/graphics/bgfx/files (https://github.com/freebsd/freebsd-ports/tree/main/graphics/bgfx/files)
And we maintain it ourselves, but prefer to push fixes upstream rather than store them locally.
You do not need to add FreeBSD to the list of official supported platforms - just push the patches is enough. :-)

[bkaradzic]

I just checked your patches, and they are actually simpler than what it would be required to maintain it in main repo. It's because you only fix what's needed for FreeBSD. Which is actually preferable IMO for someone who is using unsupported platform.

[VVD]

Of course, our patches are intended to make the application build and run on FreeBSD, not to add new features. Sometimes we find and fix bugs and try to push patches to the upstream, but this is more a task for the upstream than for the maintainers.

[solidpixel]

This patch is broken - it's truncating the 64-bit input to 32-bits before taking the pop count, so fails for most ASTC block sizes.

From my side I'm happy to support FreeBSD in the compressor upstream, as it's usually transparent, but we've long since dropped support for 32-bit x86. Nehalem is 15 years old, so I'm less convinced about supporting CPUs that old (I have no desire to re-add 32-bit testing, and I won't claim support for something I can't test).

(I'm the upstream compressor maintainer)

[VVD]

FreeBSD will support i386 ~4 years more - life cycle of the 14.x branch.

Nehalem is 15 years old

All -march on i386 newer than nehalem are affected - skylake for example too.

[bkaradzic]

@VVD Please stop referencing bgfx in FreeBSD, there is no support for FreeBSD in bgfx. It makes no sense for other people to get notified/involved about stuff you're doing in your project.