Cyles: TextureInterpolator Undefined linker behavior - performance regression.
Open, NormalPublic

Description

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?

Details

Type
Bug

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.

Bastien Montagne (mont29) triaged this task as Normal priority.Sun, Jul 22, 12:29 PM

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/