[Mesa-dev] feature levels in clover (was: Re: [PATCH 2/2] clover: Query and export int64 atomics)

[Francisco Jerez]

Jan Vesely <jan.vesely at rutgers.edu> writes:

> On Tue, 2017-09-26 at 14:51 -0700, Francisco Jerez wrote:
>> Jan Vesely <jan.vesely at rutgers.edu> writes:
>> 
>> > On Wed, 2017-09-20 at 19:10 -0500, Aaron Watry wrote:
>> > > [SNIP]
>> > > 
>> > > Not trying to rain on your parade, but I've been thinking that we
>> > > might need to be able to query the installed libclc version or
>> > > possibly consider querying the libclc library for information about
>> > > which extensions are implemented (possibly even for the specific
>> > > device/target and the llvm version it was built against).
>> > > 
>> > > I've been slowly working on a printf implementation, and I'll need to
>> > > expose that based on some dynamic state of the system as well.
>> > > 
>> > > For something that's adding new extension support in libclc, we might
>> > > at least want to bump the libclc version number and ask Tom what he
>> > > did in the past related to branching off a libclc release, and then
>> > > make sure we've got a way to link against libclc (if we're not
>> > > already) and get its version number.
>> > 
>> > I disagree.
>> > Extensions need to be exposed by both clover (for host code) and clang
>> > (for clc code), synchronizing those is tricky enough. adding two more
>> > variables (libclc version, libclc clang build version) will make things
>> > intractable.
>> > 
>> > It's a lot easier (and I'd say common) for applications to check for
>> > extensions in CLC code (ifdef/ifndef) and handle all failures in
>> > generic 'build failed' error path. That makes clang the authoritative
>> > source of supported extensions for each target.
>> > Ideally, clover would get the list of supported extensions from clang
>> > (based on device target) and expose those.
>> > clang has exposed almost all extensions since at least 3.9.
>> > Implementing and exposing them (including int64 atomics) is thus a
>> > bugfix, not a feature update.
>> > 
>> > I don't want to have libclc release until the library is complete (we
>> > still miss ~12 builtins + variants). Distros now ship development
>> > snapshot and providing a released version would make more
>> > backward^Wconservative distros (ehm, debian) stuck on that version.
>> > This way we have only one version; HEAD.
>> > 
>> > I have posted patches that bring back llvm-3.9 support to libclc, thus
>> > everyone can use the latest libclcHEAD.
>> > 
>> > I agree that we need to differentiate libclc variants that follow
>> > different version of standard. Not just because of missing features,
>> > but because not all builtins should be exposed in all versions.
>> > for example: atomic_* should not be available in clc1.0.
>> > or the program can do something like this:
>> > #ifndef extension
>> > ...custom implementation of extensions function
>> > #else
>> > #pragma extension: enable
>> > #endif
>> > 
>> > I'd thus propose to add lang version suffix to libclc.
>> > *.bc.cl100 -- follows OCL-1.0 (builtins added later are not available)
>> > *.bc.cl110 -- follows OCL-1.1 (deprecated OCL-1.0 builtins/macros are
>> > not available, buitlins from 1.2 are not available)
>> > ...
>> > Current libclc would thus provide only .cl100 and .cl110 with missing
>> > builtins/bugs.
>> > 
>> > 
>> > It'd be possible for clover to check presence of such libraries and
>> > restrict CLC (and CL) feature level accordingly.
>> > 
>> 
>> Wouldn't it be easier for Clover to check which libclc version it's
>> linking against and expose a subset of language versions and extensions
>> accordingly?  That should only take a tiny bit of build system support
>> and wouldn't lock us to a single libclc release per API version (which
>> would make things tricky if extensions get back-ported by Khronos to
>> older API versions after the fact, or if people have reasons to delay
>> the implementation of a subset of extensions after a release).
>
> I don't think libclc version makes sense.
>
> atm, there are still core clc 1.0 builtins missing so we really should
> not expose even cl1.0
>
> when it comes to extensions, libclc has to implement all builtins for
> extensions exposed by clang, whether clover exposes the same extension
> or not, otherwise legal programs fail to build.

I'd argue instead that we shouldn't be letting Clang expose an extension
that is missing support code from libclc.

> I would not expect an application to check clGetDeviceInfo for
> extensions and then pass custom defines to kernel build (that's what
> ifdef cl_khr_* is for).

Why not?  It certainly seems useful to me for the host to have an
accurate view of what extensions are supported by the language.
Sometimes working around for a missing extension implies different host
behavior (e.g. if 64bit atomics are not supported there may be a 32bit
fall-back path that requires splitting up work into smaller pieces to
avoid 32bit overflow), or providing completely different kernels.

> Moreover, libclc is the easiest one to upgrade because nothing else in
> the system depends on it (as opposed to llvm and mesa).
>
> The idea behind liblc per language variants is that the user should be
> able to use function names that were later taken for builtins (like
> shuffle).

I don't think that tracking libclc versions would be in conflict with
that.

> we might be able to get away with a single library variant if users
> don't use overloadable attribute.
>
>> 
>> Either way, patch looks good to me:
>> 
>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>
> thanks, pushed.
> Jan
>
>> 
>> > Jan
>> > 
>> > [SNIP]
>> > -- 
>> > Jan Vesely <jan.vesely at rutgers.edu>
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- 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/20170927/2c6990f6/attachment.sig>