Menu

#346 ports/cmake should not require yasm

svn
closed-fixed
nobody
None
5
2023-03-19
2022-10-25
manx
No

ports/cmake currently requires YASM with MSVC for x86 and amd64. libmpg123 is buildable without assembler just fine, so this should be an optional requirement, and not a hard requirement that cannot even be disabled.

Discussion

  • Thomas Orgis

    Thomas Orgis - 2022-10-28

    Hm. But using assembly optimizations is a major point of libmpg123, it being faster than alternatives without. Of course, we can make this optional in the MSVC build, but it should be encouraged.

    Do you think it would be sensible to have the default build use yasm and cmake error out if it is not found, with a message that one could explicitly choose the generic decoder without assembly?

    There has been another request about converting the assembly to inline asm that works with gcc. I am no fan of that … but it might be relevant if MSVC, clang, Intel, etc. agrees on the format for inline assembly and one would get away without the need for yasm / gas.

    But then, I am really not fond of making that transition. Maybe the assembly opts do become irrelevant at some point.

    A data point about benefits of assembly files: https://mpg123.org/benchmark/mpg123-1.31.0_Core-i5-8250U%401600MHz_gcc-9.4.0.txt

    #mpg123 benchmark (user CPU time in seconds for decoding)
    #decoder    t_s16/s t_f32/s
    AVX 2.20    2.14
    x86-64  2.34    2.27
    generic 3.29    4.73
    generic_dither  3.38    4.74
    

    So, even with gcc 9.4, the generic C code is still only half as efficient as the hand-tuned x86-64 (SSE), with even a bit more edge for the AVX variant. Somehow sad how you still cannot expect automatic use even of SSE from compiled code. Peak flops are so irrelevant unless you hand-craft your vectorization. Intrinsics might work nowadays, though.

    Yeah, you could try to port the SSE/AVX code to C with vector intrinsics. Does MSVC support those in a portable manner? ;-)

     

    Last edit: Thomas Orgis 2022-10-28
    • manx

      manx - 2022-10-28

      Do you think it would be sensible to have the default build use yasm and cmake error out if it is not found, with a message that one could explicitly choose the generic decoder without assembly?

      Not really. Almost all users of Visual Studio do not have yasm installed, and they would thus not be able to build a default configuration. That's basically equivalent to not really supporting Visual Studio at all.
      I think the default build should not rely on additional tools if possible. At best, it could print a warning due to missing asm optimizations, but not an error.

      There has been another request about converting the assembly to inline asm that works with gcc. I am no fan of that … but it might be relevant if MSVC, clang, Intel, etc. agrees on the format for inline assembly and one would get away without the need for yasm / gas.

      Yeah, that makes no sense in the context of MSVC. In fact, it makes the situation worse, because it makes using yasm impossible.
      GCC/Clang/Intel support GCC-style inline asm, MSVC/Clang/Intel support MSVC-style inline asm, so one would need to implement at least 2 variants.
      Also, amd64 MSVC just does not support any inline asm at all, so this is really not an option, IMHO.

      Yeah, you could try to port the SSE/AVX code to C with vector intrinsics. Does MSVC support those in a portable manner?

      Yes, MSVC supports x86/amd64 intrinsics just fine, and that's what I would suggest as a long-term solution. There is probably no need to touch code for old CPUs though (i.e. anything before SSE2 can surely just stay as is) as these are not officially supported by Windows any more anyway.

      There were recently also somewhat related discussions in FLAC: https://github.com/xiph/flac/pull/452 and https://github.com/xiph/flac/issues/486. FLAC was already mostly intrinsics, and now all remaining asm has been removed.

       
  • Thomas Orgis

    Thomas Orgis - 2022-10-28

    Hm. I could just not care what MSVC does … but I wonder how we properly communicate that you really should consider getting yasm when building libmpg123. It's just wasteful not to use the AVX optimization (or ARM Neon).

     
  • manx

    manx - 2022-10-29

    If you want people to use something, you make it work by default without any user intervention required at all.
    The way to achieve that for MSVC is porting the asm to intrinsics.

    Expecting people to install yasm just to built an absolutely not performance critical (on modern hardware) library for a single file format is just not realistic at all. I even question if a warning would be that useful. The majority of users will just ignore the warning or silence it, instead of complicating their build. I certainly will (and I have always done so for OpenMPT and libopenmpt).

     
  • Thomas Orgis

    Thomas Orgis - 2022-10-29

    Another point about intrinsics: I am not aware of a compiler-agnostic way to get cpuid information to dispatch SSE or AVX code (intrinsics code with certain vector size, other variations that matter) … there seem to be compiler-specific checks for supported instructions. But porting the actual optimized routines doesn't gain much if one still has to resort to plain assembly to identify the CPU features.

    One solution would be to just assume SSE on x86-64 and nothing more, but this is hardly optimal, while the tradeoff is a lot smaller than doign without any optimization.

     
  • manx

    manx - 2022-10-29

    I am not aware of a compiler-agnostic way to get cpuid information to dispatch SSE or AVX code [...]
    But porting the actual optimized routines doesn't gain much if one still has to resort to plain assembly to identify the CPU features.

    cpuid needs some (very limited) compiler-specific implementation, but nothing serious. All compilers have intrinsics or helpers for cpuid: __cpuid/__cpuidex for MSVC, __cpuid/__cpuid_count for GCC/Clang, and for Intel and GCC there are higher level abstractions like _may_i_use_cpu_feature or __builtin_cpu_supports.

    One solution would be to just assume SSE on x86-64 and nothing more, but this is hardly optimal, while the tradeoff is a lot smaller than doign without any optimization.

    You can also detect the ISA extensions that are assumed by the compiler by detecting the macros set by -march= or /arch: in the source. That way, -msse2 for x86 would also be able to use SSE2 intrinsics without cpuid.

    Honestly, I do not care much if you want to invest the time to port the asm to intrinsics or not.

    In any case, purposefully failing the build just because you personally do not like the performance of the resulting binary is never the right choice, IMO.

     
  • Thomas Orgis

    Thomas Orgis - 2023-01-15

    I hacked a bit to fall back to generic decoder and emit a warning without yasm in revision 5210. Can you confirm that it works? Hm, maybe our github mirror CI will pick up the added workflow, too.

     
  • manx

    manx - 2023-01-21

    CMake/YASM change looks good and works for me.

    The GitHub mirror appears to not update at all at the moment.

     
  • Thomas Orgis

    Thomas Orgis - 2023-03-19
    • status: open --> closed-fixed
     

Log in to post a comment.