OpenCL-CTS icon indicating copy to clipboard operation
OpenCL-CTS copied to clipboard

basic: Add volatile to prevent optimizations

Open lakshmih opened this issue 10 months ago • 3 comments

Prevent the compiler from optimizing away initialization loops

lakshmih avatar Apr 07 '24 06:04 lakshmih

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).

svenvh avatar Apr 08 '24 09:04 svenvh

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.

bashbaug avatar Apr 19 '24 21:04 bashbaug

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 avatar May 07 '24 16:05 lakshmih

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)

svenvh avatar May 08 '24 08:05 svenvh

Merging as discussed in the May 21st teleconference.

bashbaug avatar May 21 '24 15:05 bashbaug