[Mesa-dev] [RFC] opencl: mega-cl

[Francisco Jerez]

Rob Clark <robdclark at gmail.com> writes:

> On Sun, Feb 25, 2018 at 3:00 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Seems like a serious hack to me to work around broken linking...  IMO we
>> should just fix the linking issue.  The symbols for the various GLSL
>> types need to be linked with the proper binding and visibility -- I
>> assume that the cause of your problem is that people are making
>> assumptions about the equality of GLSL types based on their memory
>> addresses *and* marking the symbols as hidden *and* passing pointers to
>> GLSL types across shared objects?  That sounds like a recipe for
>> disaster.
>
> tbh, maybe hack, or I think more likely, maybe a good idea.. I'm not
> terribly sold on the idea of dynamically loading pipe driver and
> linking a lot of shared code into N different pipe_${driver}.so on
> disk, since the # of drivers seems to be greater than # of state
> trackers.. not to mention multiple copies of shared gallium code in
> memory due to being statically linked into both state tracker and
> driver.
>

It would probably make sense to link that shared code dynamically into
the pipe-loader modules in order to avoid the duplication you're talking
about (e.g. by building them as a meson shared_module() instead of a
shared_library() in order to allow the common symbols to be undefined in
the pipe-loader module which will then be pulled in by the state
tracker) -- Actually that's likely to fix your problem too.

> That said, glsl_type's defn does seem to expect to only exist once in
> a process, ie. == is ptr comparison and when you link nir/glsl_types
> into both state tracker and driver, glsl_type ptrs are getting passed
> across that boundary.  I'm not really sure that is worth fixing (ie.
> why should it exist twice in a process in the first place?)
>

*Shrug* It's kind of a micro-optimization with nasty side effects
(namely that you cannot pass GLSL IR or NIR across shared object
boundaries without things exploding) and dubious payoff, it would
probably be reasonable to simply drop the micro-optimization, but that
wasn't the solution I was suggesting (but rather, making the
optimization work by exporting the symbols GLSL type equality cares
about with the right linkage).

> Maybe there are some linker tricks to solve this, idk.. that is a bit
> outside my area of expertise.

More than adding new linker tricks I think you just need to make sure
that existing linker tricks we're doing right now ('-fvisibility=hidden'
and various linker scripts) don't break the build by forcing common
symbols to be local, along the lines of:

| diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
| index ab0b2637649..9d8a5ea341c 100644
| --- a/src/compiler/glsl_types.h
| +++ b/src/compiler/glsl_types.h
| @@ -243,7 +243,7 @@ public:
|     /*@{*/
|  #undef  DECL_TYPE
|  #define DECL_TYPE(NAME, ...) \
| -   static const glsl_type *const NAME##_type;
| +   static const glsl_type *const NAME##_type __attribute__((visibility("default")));
|  #undef  STRUCT_TYPE
|  #define STRUCT_TYPE(NAME) \
|     static const glsl_type *const struct_##NAME##_type;
| diff --git a/src/gallium/targets/opencl/opencl.sym b/src/gallium/targets/opencl/opencl.sym
| index 9fcc57692b8..fadc2eb6c1d 100644
| --- a/src/gallium/targets/opencl/opencl.sym
| +++ b/src/gallium/targets/opencl/opencl.sym
| @@ -2,6 +2,10 @@
|  	global:
|  		cl*;
|  		opencl_dri_*;
| +
| +		extern "C++" {
| +			glsl_type::*;
| +		};
|  	local:
|  		*;
|  };
| diff --git a/src/gallium/targets/pipe-loader/pipe.sym b/src/gallium/targets/pipe-loader/pipe.sym
| index 605cb83d802..801b0f44d56 100644
| --- a/src/gallium/targets/pipe-loader/pipe.sym
| +++ b/src/gallium/targets/pipe-loader/pipe.sym
| @@ -7,6 +7,10 @@
|  		# due to LLVM being initialized multiple times.
|  		radeon_drm_winsys_create;
|  		amdgpu_winsys_create;
| +
| +		extern "C++" {
| +			glsl_type::*;
| +		};
|  	local:
|  		*;
|  };

> To me, the mega-driver approach worked well for gl, and many of the
> other state trackers are already using libpipe_loader_static.. why not
> just fix the problems with that approach and abandon
> libpipe_loader_dynamic?  This seems like it would work out better for
> memory footprint since pages for drivers not used simply wouldn't get
> paged in.  And you aren't ending up with two copies of libgallium or
> libnir code in memory.
>

The dynamic pipe loader would give you better disk and memory footprint
than the static one assuming that the problem with linking shared code
statically into each pipe-loader module was fixed, because that would
allow the pipe driver code to be shared in memory and disk among
different state trackers, but I guess that the benefit is slight at this
point, I understand your reluctance to fix it ;).

> BR,
> -R
>[...]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180226/b50e5984/attachment.sig>