flac dev - May 2016 - [PATCH] workaround for a bug in MSVC 2015 U2

Erik de Castro Lopo wrote:
>> As I wrote earlier, MSVC 2015 U2 incorrectly compiles
>> stream_encoder_intrin_*.c files for x86-64 platform.
>> As a result, flac works, but compression ratio is close to 1.
>> This patch disables some compiler optimizations, and
>> compression ratio reverts back to normal values.
>
> Rather than having the same chunk of code in three different
> files, wouldn't it be nicer to put it in "share/compat.h" and
> include it as needed?
But it will disable optimization on all other files that directly or
indirectly include compat.h (that is, almost all files).

IMHO it's too broad. It seems that this problem with MSVC is local
and only stream_encoder_intrin_*.c are affected.



Or do you mean something like that --

share/compat.h:

     #if defined FLAC_ENABLE_MSVC2015U2_WORKAROUND1
     #if (defined _MSC_VER) && (_MSC_FULL_VER == 190023918) &&
(defined FLAC__CPU_X86_64)
     #pragma optimize("g", off) /* workaround for a bug in MSVC 2015
U2 */
     #endif
     #endif

src/libFLAC/stream_encoder_intrin_*.c:

     #define FLAC_ENABLE_MSVC2015U2_WORKAROUND1
     #include "share/compat.h"

-- ?

...Not sure that share/compat.h is a right place for this workarounds.
This bug is quite local and doesn't manifest itself in other source
files, and share/compat.h is quite global and affects many files.

lvqcl wrote:
> ...Not sure that share/compat.h is a right place for this workarounds.
> This bug is quite local and doesn't manifest itself in other source
> files, and share/compat.h is quite global and affects many files.
Ok, I'll take the original patch as it is. DO you have a link to a
bug report on this problem? It would be nice to put it in a comment
above the workaround.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Erik de Castro Lopo wrote:
> Ok, I'll take the original patch as it is. DO you have a link to a
> bug report on this problem? It would be nice to put it in a comment
> above the workaround.
:sigh: I hoped that somebody else will find what's wrong and will create a
bugreport...

Well, here is the link:  
https://connect.microsoft.com/VisualStudio/feedback/details/2659191/incorrect-code-generation-for-x86-64


It seems that MSVC miscompiles
     abs_residual_partition_sums[partition] =
(FLAC__uint32)_mm_cvtsi128_si32(mm_sum);
into
     movq    QWORD PTR [rsi], xmm2

So it incorrectly copies 8 bytes from mm_sum to
abs_residual_partition_sums[partition]
(it should copy 4 lower bytes and zero out 4 upper bytes).

It should be something like
     movd    eax, xmm2
     movq    QWORD PTR [rsi], rax