RenderKit / ospray

An Open, Scalable, Portable, Ray Tracing Based Rendering Engine for High-Fidelity Visualization
http://ospray.org
Apache License 2.0
1.02k stars 186 forks source link

compilation error on cray w/ icc 18.0.5 #331

Closed demarle closed 3 years ago

demarle commented 5 years ago

I'm getting an error on some Cray builds with icc 18.0.5. Error is: error: cannot convert 'float ' to 'const float ()[8]' for argument '4' to 'void ispc::LocalFrameBuffer_accumulateAuxTile(...

changing LocalFB.ispc's definition from

export void LocalFrameBuffer_accumulateAuxTile(...
, const varying float * uniform ax
, const varying float * uniform ay
, const varying float * uniform az)

to

export void LocalFrameBuffer_accumulateAuxTile(...
, const float * uniform ax
, const float * uniform ay
, const float * uniform az)

Gets me by this compilation problem but I don't understand the consequences of that.

Is the above change kosher?

jeffamstutz commented 5 years ago

I think it would be legal if you also changed:

uniform uint32 chunkID = (iiy-tile.region.lower.y)*(TILE_SIZE/programCount);

...to

uniform uint32 chunkID = (iiy-tile.region.lower.y)*(TILE_SIZE);

The addressing computations assume chunks of programCount size of pixels, so working on plain float (uniform float) instead of vectors of float (varying float) would need chunkID to line up correctly. Not sure the performance impact, however.

You could probably change the parameters to be const void *uniform and cast them to const varying float *uniform, as the function interface exported to C++ doesn't need to have the varyingness of the data encoded there....only within the ISPC function itself.

jeffamstutz commented 5 years ago

Regardless, we can patch this in devel for our next release.

demarle commented 5 years ago

Thanks. Casting also compiles on the system in question so I'm going with that. I'll add a patchset to paraview superbuild as it will be on ospray 1.8.x for the next few months.

demarle commented 5 years ago

Undergoing testing here: https://gitlab.kitware.com/paraview/paraview-superbuild/merge_requests/605

johguenther commented 5 years ago

This will result in wrong ISPC code, because float * uniform is uniform float * uniform, which is different to the original varying float * uniform.

What ISPC version do you use? I tested with v1.10, v1.11 and the latest git-master, and they all generate build/ospray/LocalFB_ispc.h:

...
#if defined(__cplusplus) && (! defined(__ISPC_NO_EXTERN_C) || !__ISPC_NO_EXTERN_C )
extern "C" {
#endif // __cplusplus
    extern void LocalFrameBuffer_accumulateAuxTile(void * _fb, const struct Tile &tile, struct vec3f * aux, void * ax, void * ay, void * az);
...

i.e. a void * which can be casted to automatically in the call from LocalFB.cpp.

johguenther commented 5 years ago

I just see in your patch that you directly use void * and cast inside the ISPC function, which should indeed to the trick. Still, I wonder why this is suddenly necessary in the first place.

demarle commented 5 years ago

icc 18.0.5, ispc 1.9.2, embree 3.2.0, ospray 1.8.4 all within craype 2.5.6 on trinity's little brother trinitite.

johguenther commented 3 years ago

fixed long ago