Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

basic: workaround MSVC compiler bug with post-increment operator #1939

Merged
merged 1 commit into from
May 21, 2024

Conversation

lakshmih
Copy link
Contributor

@lakshmih lakshmih commented Apr 7, 2024

Prevent the compiler from optimizing away initialization loops

@svenvh
Copy link
Member

svenvh commented Apr 8, 2024

Disabling optimizations is not what volatile is intended for. Are you able to provide more details about what's going wrong? I've run this test with ubsan and asan just fine, so perhaps it's a miscompilation of your compiler? If so, it would be better to disable optimizations for a specific compiler (with gcc/clang you could use #pragma GCC optimize("-O0") for example, together with push_options / pop_options to keep it local to a function).

@bashbaug
Copy link
Contributor

Discussed in the April 16th teleconference. We’d prefer not to use “volatile” to work around this issue. Qualcomm will include more information about the failing case, e.g. which compiler is exhibiting the problem.

@lakshmih
Copy link
Contributor Author

lakshmih commented May 7, 2024

This is with Visual Studio compiler VS 2022
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\bin\Hostx86\x86\cl.exe

It is probably a compiler bug. It also reproduces pretty consistently in a unit test that just calls that function from main like below:

int main()
{
    std::vector<float> reference(576);
    makeReference<float, 16, 16>(reference);
    for (auto v : reference) {
        printf("%f\n", v);
    }
}

Disabling optimizations via pragma just for that function does not help.

We have precedence for using volatile to prevent optimizations (including the commented instance here https://github.com/KhronosGroup/OpenCL-CTS/blob/main/test_conformance/basic/test_local_kernel_scope.cpp#L114) but if we would prefer to, switching the variable to post-increment seems to help. Would that be acceptable?

--- a/test_conformance/basic/test_vector_swizzle.cpp
+++ b/test_conformance/basic/test_vector_swizzle.cpp
@@ -516,8 +516,7 @@ static void makeReference(std::vector<T>& ref)
     // single channel lvalue
     for (size_t i = 0; i < N; i++)
     {
-        ref[dstIndex * S + i] = 0;
-        ++dstIndex;
+        ref[dstIndex++ * S + i] = 0;
     }

This is breaking conformance for us, so would like to merge some manner of fix for it.

@lakshmih lakshmih requested a review from svenvh May 7, 2024 18:24
@svenvh
Copy link
Member

svenvh commented May 8, 2024

Thanks for the detail. If it fixes your issue, switching to post-increment would be fine with me. Otherwise, I suppose you can add something like this to the test's CMakeLists.txt:

if (MSVC)
  set_source_files_properties(test_vector_swizzle.cpp PROPERTIES COMPILE_FLAGS "/O0")
endif(MSVC)

@bashbaug bashbaug changed the title basic: Add volatile to prevent optimizations basic: workaround MSVC compiler bug with post-increment operator May 21, 2024
@bashbaug
Copy link
Contributor

Merging as discussed in the May 21st teleconference.

@bashbaug bashbaug merged commit 5ce18c3 into KhronosGroup:main May 21, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants