thakis@/dalecurtis@: Is this truly specific to just Windows? Or do linux/mac configs also need munging?

Note that https://chromium-review.googlesource.com/#/c/344671/ may actually fix this bug (following route #2 or #3). It's iterating in CR right now.

Since configure probes the local system, it probably applies to non-Windows too -- but the intrinsic setup on non-Windows until very recently was so that you could only use e.g. SSE intrinsics if you compiled your code with a --target cpu that supports SSE. On Windows, it's historically possible to use e.g. SSE intrinsics in files where the compiler isn't allowed to use SSE for the code it creates, and so it's easier to (accidentally and intentionally) mix-and-match SSE and non-SSE code in one TU. So it's probably more pressing on Windows.

https://chromium-review.googlesource.com/#/c/344671/ does (2) for now (but we can probably do better later).

Ok. I'll keep this bug open for possibly implementing (3) (for ia32 and x64 windows/linux/mac/etc) in the future. Once https://chromium-review.googlesource.com/#/c/344671 lands with implementation of (2), this bug will no longer block  bug 591845 .

Sounds good. Please let me know if the --cpu flag causes any issues. Happy to help in that case.

@#4: It landed and rolled (https://chromium.googlesource.com/chromium/src/+/476f6c9166db498c42887fb78229b6de54f6e148), so this bug no longer blocks  bug 591845 .

Keeping open for possibly implementing (3) in future.

=> hubbe@ for potentially implementing (3) in the M-57 roll.

=> liberato's doing the M-57 roll instead.

xhwang@ - Is this something you could fix during your M65 roll? (If not, please remark as blocking the next ffmpeg roll beyond yours).

Assign to sandersd@ since the ffmpeg rotation has shifted and now sandersd@ will work on the M65 roll.

ffmpeg uses runtime cpu detection for most things, so we're mostly just worried about this one edge case with the MSVC specific intrinsic. For this case does anyone remember why we didn't just check defined(__BMI__) ?

Seems no __BMI__ in MSVC, and even with clang -mavx2 doesn't seem to imply -mbmi; so that's a bit surprising. In any case, looking over the discussion on cfe-dev@ hans says that BMI isn't required for _tzcnt_*:

http://lists.llvm.org/pipermail/cfe-dev/2016-October/051331.html

...but that still isn't fixed on trunk it seems:

../../third_party/ffmpeg/libavcodec/h2645_parse.c(202,17):  error: implicit declaration of function '_tzcnt_u32' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        size -= ff_ctz(v) + 1;

I think a simple patch to x86/intmath.h to only define these when !__clang__ would be upstreamable. Will send it and see.

I don't remember details, but I think on our end we concluded that what ffmpeg is doing is really questionable and we shouldn't change the compiler. So if you're changing ffmpeg upstream, maybe you could make things just not behave weirdly. Hans, do you remember what that meant concretely?

Happy to try if it's truly questionable, but we don't see any crashes in this code prior to your fix (and the number of opterons is non-zero), so I think hans@ is right:

https://crash.corp.google.com/browse?q=product.name%20CONTAINS%20%27Chrome%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.file_path%20CONTAINS%20%27ffmpeg%2Flibavutil%2Fmathematics.c%27%20AND%20product.Version%20CONTAINS%20%2751.0.%27&sql_dialect=dremelsql&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D

Either we don't have any Opteron users or it would seem either _tzcnt_* maps correctly on non-BMI hardware with MSVC.

For historical context, hans@ originally landed http://llvm.org/viewvc/llvm-project?view=revision&revision=253358 based on this discussion on ffmpeg-devel:  https://ffmpeg.org/pipermail/ffmpeg-devel/2015-November/183404.html -- but I guess it got reverted at some point.

There was a later attempt by thakis@ as part of https://bugs.llvm.org/show_bug.cgi?id=30506, but it appears this never landed.

Per the e-mail it seems pretty clear ffmpeg is okay with the "rep bsf" encoding tzcnt degrades to (and answers my question about why we didn't have crashes).

Probably the best I can do from the ffmpeg side is to add a __clang__ #define if clang-cl isn't planning to match MSVC behavior here. 

Sent http://ffmpeg.org/pipermail/ffmpeg-devel/2017-October/218445.html upstream which I think is as much as I can get away with.

Per the discussion on https://reviews.llvm.org/D26335#inline-227758 , iirc the argument went the rep bsf doesn't check for 0, so if you really want tzcnt you need to check for 0 yourself first, and at that point it really doesn't buy you anything to use the fallback encoding. So it's not clear that they get any perf wins from what they're currently doing (and I think the commit message that added that code was like "might as well..." and not anything rigorous)

Upstream took my patch http://git.videolan.org/?p=ffmpeg.git;a=commit;h=50e30d9bb71e1dff27be16c264fac90e362b9896 so we can drop the cpu switch with the next ffmpeg roll.

Fixed in latest roll. No more cpu=opteron and all builds are done with Clang, so we shouldn't get any bad flags again.