Fix fast_exp2 with MSVC and sse4.2

[ericmehl]

Description

I ran into the problem described in #2929 and did a little digging to see if I could find the source of the bad color conversions. I tracked it down to fast_exp2() https://github.com/OpenImageIO/oiio/blob/a77e36250fadfb4c391688eb1df4b3dd88d5e367/src/include/OpenImageIO/fmath.h#L2050-L2080

and in particular the return statement on L2071. In the process of trying to figure out what part of that return statement was going awry, I found that separating the bitcasts to separate variables as in this PR, the problem went away. All testing was done with the color_test added in #3023.

It also succeeded when adding various std::cout lines to check what the values of each computation were, so it does sound like a compiler optimization that does or does not get triggered is the problem.

Unfortunately I don't know why it's not failing on the CI builds, I double checked to make sure I'm using the same settings and flags. I also tested on two different machines and get the same failures, and success with this patch, on both.

One strange wrinkle is that even without this patch, color conversions worked fine when using AVX2 for the SIMD mode.

Summary :
Previous results (without this patch)

• USE_SIMD=0 - PASS
• USE_SIMD=SSE4.2 - FAIL
• USE_SIMD=AVX - FAIL
• USE_SIMD=AVX2 - PASS

With this patch

• USE_SIMD=0 - PASS
• USE_SIMD=SSE4.2 - PASS
• USE_SIMD=AVX - PASS
• USE_SIMD=AVX2 - PASS

Visual Studio 2019 versions 16.11.32228.343, 16.11.33328.57 and 16.11.33423.256(same as current Github build runners) in RelWithDebInfo mode.

Checklist:

• I have read the contribution guidelines.
• If this is more extensive than a small change to existing code, I
  have previously submitted a Contributor License Agreement
  (individual, and if there is any way my
  employers might think my programming belongs to them, then also
  corporate).
• I have updated the documentation, if applicable.
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• My code follows the prevailing code style of this project.

[lgritz]

How strange!

Can I request that you please add a test to fmath_test.cpp (perhaps in test_math_functions()) using specific values that would fail without the fix, and succeed with it?

[lgritz]

This is quite an obscure problem, I appreciate your taking the time to track it down to a single line and produce a fix.

[ericmehl]

This is quite an obscure problem, I appreciate your taking the time to track it down to a single line and produce a fix.

My pleasure! It's been a strange journey.

Can I request that you please add a test to fmath_test.cpp (perhaps in test_math_functions()) using specific values that would fail without the fix, and succeed with it?

I tried adding a test for fast_exp2() but I couldn't get it to fail on the unpatched code. Even when a test of linear_to_sRGB() and sRGB_to_linear() directly before it failed. I also tried pasting in the code for the return value of https://github.com/OpenImageIO/oiio/blob/3332c78f5eacd65c622f97941c2249318b395db9/src/include/OpenImageIO/color.h#L374-L379

and that passed. So it seems to be unique to the case of being called / inlined into the linear_to_sRGB() and sRGB_to_linear() functions.

Does that suggest any new solutions or other avenues to investigate?

[johnhaddon]

Thanks for merging @lgritz. Do you think this could be backported for the next 2.4 release (it fixes a problem with black icons in Gaffer on Windows)?

[lgritz]

Certainly. Do you need a tagged release right away, or is the scheduled May 1 patch good enough?

…

On April 17, 2023 1:50:34 a.m. PDT, John Haddon ***@***.***> wrote: Thanks for merging @lgritz. Do you think this could be backported for the next 2.4 release (it fixes a problem with black icons in Gaffer on Windows)? -- Reply to this email directly or view it on GitHub: #3804 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[johnhaddon]

Thanks! May 1st will be perfectly sufficient - we have a workaround in the meantime.

[lgritz]

I already had merged it into the head of dev-2.4, by the way. The only thing it's lacking is a release (and whatever else I may add between now and the end of the month.