Cyles: TextureInterpolator Undefined linker behavior - performance regression. #55054

New Issue

Ray molenkamp · 2018-05-13T21:56:12+02:00

Ray molenkamp commented

2018-05-13 21:56:12 +02:00

Allright, while implementing clang support in D3304 I ran into an issue where
I'd hit an andn BMI instruction (haswell and up, my ivy lake didn't take that too well)

And it took me a few days to figure out, and it's gonna take quite a bit of explaining
to get to the cause of the bug.

lets fist take some detours to get some basic concepts ironed out.

Detour number 1: Undefined behavior - Libraries and duplicate functions

/* ===== a.cpp ===== */ 

#include <stdio.h>

void MyFunc()
{
	printf("Hello from A\n");
}

/* ===== b.cpp ===== */ 

#include <stdio.h>

void MyFunc()
{
	printf("Hello from B\n");
}

/* ===== main.cpp ===== */ 

#include <stdio.h>

extern void MyFunc();
int main(int argc, char**argv)
{
	MyFunc();
}

/* ===== CMakelists.txt ===== */ 

cmake_minimum_required(VERSION 2.8)
project(libtest)
add_library(mytest a.cpp b.cpp)
add_executable(maintest main.cpp)
target_link_libraries(maintest mytest)

Seems simple enough. Question time, what does this do?

a) Error out at link time, duplicate functions aren't allowed!
b) print "Hello From A"
c) print "Hello From B"
d) print "Hello From A" on windows, "Hello From B" on linux

turns out the answer is d. If there's multiple definitions in a library the linker is apparently
allowed to pick 'whatever it feels like' and not tell anyone about it, on linux it seems to
depend on the link order, microsoft's linker ... errr..yeah....I have honestly no idea what the logic is.

Detour number 2, behavior of the static keyword in C++

when using the static keyword on a c++ class it does NOT limit the visiblity to just
this compile unit like it does in C, it merely marks if you can call this function
without an instance (there's more to it, but given our usage of it, this will suffice)

Bringing it all together:

in kernel_cpu_image.h there's this little snippet

#ifdef __GNUC__
  static ccl_always_inline
#else
  static ccl_never_inline
#endif
  float4 interp_3d_tricubic(const TextureInfo& info, float x, float y, float z)
  {

the never_inline is there, cause the massive amount of inlining added 20 minutes to a cycles release build
with msvc. we put this there before it was a templated class so that worked, however when this got refactored
into a template, wonkyness ensured.

At this point, every kernel compile unit will have a interp_3d_tricubic function, the linker is free
to pick any of them, and when build with msvc will consistently pick the unoptimized one from the 'cpu'
kernel, however when build with clang, it went with the AVX2 implementation causing a segfault with an
invalid instruction.

Current Situation:

when build with msvc, interp_3d_tricubic will use the unoptimized cpu implementation
when build with clang it'll use the avx2 implementation and segfault on all systems that have no avx2.

Solution:

I have nothing pretty really, but here's some options.

template<typename T> struct KERNEL_FUNCTION_FULL_NAME(TextureInterpolator) {

could work, but it requires hackery in osl_services.cpp since it doesn't know about the KERNEL_FUNCTION_FULL_NAME macro

We could remove the ccl_never_inline but it'll balloon the build time, also i'm not convinced the linker will do
the the right thing in a debbug build with inlining disabled.

anyone else have a bright idea here?

Allright, while implementing clang support in [D3304](https://archive.blender.org/developer/D3304) I ran into an issue where I'd hit an andn BMI instruction (haswell and up, my ivy lake didn't take that too well) And it took me a few days to figure out, and it's gonna take quite a bit of explaining to get to the cause of the bug. lets fist take some detours to get some basic concepts ironed out. **Detour number 1: Undefined behavior - Libraries and duplicate functions** ``` /* ===== a.cpp ===== */ #include <stdio.h> void MyFunc() { printf("Hello from A\n"); } /* ===== b.cpp ===== */ #include <stdio.h> void MyFunc() { printf("Hello from B\n"); } /* ===== main.cpp ===== */ #include <stdio.h> extern void MyFunc(); int main(int argc, char**argv) { MyFunc(); } /* ===== CMakelists.txt ===== */ cmake_minimum_required(VERSION 2.8) project(libtest) add_library(mytest a.cpp b.cpp) add_executable(maintest main.cpp) target_link_libraries(maintest mytest) ``` Seems simple enough. Question time, what does this do? a) Error out at link time, duplicate functions aren't allowed! b) print "Hello From A" c) print "Hello From B" d) print "Hello From A" on windows, "Hello From B" on linux turns out the answer is d. If there's multiple definitions in a library the linker is apparently allowed to pick 'whatever it feels like' and not tell anyone about it, on linux it seems to depend on the link order, microsoft's linker ... errr..yeah....I have honestly no idea what the logic is. **Detour number 2, behavior of the static keyword in C++** when using the static keyword on a c++ class it does NOT limit the visiblity to just this compile unit like it does in C, it merely marks if you can call this function without an instance (there's more to it, but given our usage of it, this will suffice) **Bringing it all together:** in kernel_cpu_image.h there's this little snippet ``` #ifdef __GNUC__ static ccl_always_inline #else static ccl_never_inline #endif float4 interp_3d_tricubic(const TextureInfo& info, float x, float y, float z) { ``` the never_inline is there, cause the massive amount of inlining added 20 minutes to a cycles release build with msvc. we put this there before it was a templated class so that worked, however when this got refactored into a template, wonkyness ensured. At this point, every kernel compile unit will have a interp_3d_tricubic function, the linker is free to pick any of them, and when build with msvc will consistently pick the unoptimized one from the 'cpu' kernel, however when build with clang, it went with the AVX2 implementation causing a segfault with an invalid instruction. **Current Situation:** - when build with msvc, interp_3d_tricubic will use the unoptimized cpu implementation - when build with clang it'll use the avx2 implementation and segfault on all systems that have no avx2. **Solution:** I have nothing pretty really, but here's some options. ``` template<typename T> struct KERNEL_FUNCTION_FULL_NAME(TextureInterpolator) { ``` could work, but it requires hackery in osl_services.cpp since it doesn't know about the KERNEL_FUNCTION_FULL_NAME macro We could remove the ccl_never_inline but it'll balloon the build time, also i'm not convinced the linker will do the the right thing in a debbug build with inlining disabled. anyone else have a bright idea here?

Ray molenkamp commented

2018-05-13 21:56:12 +02:00

Added subscriber: @LazyDodo

Brecht Van Lommel was assigned by Ray molenkamp

2018-05-13 21:56:39 +02:00

Ray molenkamp commented

2018-05-13 21:56:39 +02:00

Added subscriber: @Sergey

Sergey Sharybin commented

2018-05-13 22:14:11 +02:00

Proper way to deal with this would be to move every CPU kernel into a namespace. That would ensure linker never re-uses "wrong" microarchitecture implementation.

Germano Cavalcante commented

2018-05-13 23:28:44 +02:00

Added subscriber: @mano-wii

Ray molenkamp commented

2018-07-25 01:48:14 +02:00

looked some more into this, looks like chrome ran into a similar issue coming from math.h, all i can say is 'yikes'

https://bugs.chromium.org/p/chromium/issues/detail?id=666707

https://randomascii.wordpress.com/2016/12/05/vc-archavx-option-unsafe-at-any-speed/

looked some more into this, looks like chrome ran into a similar issue coming from math.h, all i can say is 'yikes' https://bugs.chromium.org/p/chromium/issues/detail?id=666707 https://randomascii.wordpress.com/2016/12/05/vc-archavx-option-unsafe-at-any-speed/

blender-admin commented

2019-08-16 16:27:15 +02:00

This issue was referenced by blender/cycles@9933b10a15

This issue was referenced by blender/cycles@9933b10a15c251fe34cbdd6a07743b4830c6ad8e

blender-admin commented

2019-08-16 16:27:15 +02:00

This issue was referenced by ea8e0df672

This issue was referenced by ea8e0df672713a7b3ff408242c940e1f5e78c9c1

Brecht Van Lommel commented

2019-08-16 16:49:29 +02:00

Changed status from 'Open' to: 'Resolved'

Brecht Van Lommel closed this issue

2019-08-16 16:49:29 +02:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Cyles: TextureInterpolator Undefined linker behavior - performance regression. #55054