[Mesa-dev] [PATCH 12/59] mesa: Add support for 64-bit integer uniforms. (v2)

[Nicolai Hähnle]

On 26.10.2016 02:59, Ian Romanick wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> This hooks up the API to the internals for 64-bit integer uniforms.
>
> v2: update to use non-strict aliased alternatives
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/mesa/main/uniform_query.cpp |  82 ++++++++++++++++++-
>  src/mesa/main/uniforms.c        | 170 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 247 insertions(+), 5 deletions(-)
>
> diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp
> index db700df..8ecaef4 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
[snip]
> @@ -416,6 +433,22 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>                    memcpy(&dst[didx].f, &tmp, sizeof(tmp));
>  		  break;
>                 }
> +               case GLSL_TYPE_UINT64: {
> +                  uint64_t tmpu;
> +                  double tmp;
> +                  memcpy(&tmpu, &src[sidx].u, sizeof(tmpu));
> +                  tmp = tmpu;
> +                  memcpy(&dst[didx].f, &tmp, sizeof(tmp));
> +                  break;
> +               }
> +               case GLSL_TYPE_INT64: {
> +                  int64_t tmpi;
> +                  double tmp;
> +                  memcpy(&tmpi, &src[sidx].i, sizeof(tmpi));
> +                  tmp = tmpi;
> +                  memcpy(&dst[didx].f, &tmp, sizeof(tmp));
> +                  break;

This pattern really looks quite nasty. Can we at least not pretend that 
the .f is meaningful and use

   memcpy(&dst[dix], ...);

or maybe even *(double *)&dst[didx] = tmpi;?


> +               }
>  	       default:
>  		  assert(!"Should not get here.");
>  		  break;
> @@ -453,12 +486,45 @@ _mesa_get_uniform(struct gl_context *ctx, GLuint program, GLint location,
>                    dst[didx].i = IROUNDD(tmp);
>  		  break;
>                 }
> +               case GLSL_TYPE_UINT64: {
> +                  uint64_t tmp;
> +                  memcpy(&tmp, &src[sidx].u, sizeof(tmp));
> +                  dst[didx].i = tmp;
> +                  break;
> +               }
> +               case GLSL_TYPE_INT64: {
> +                  int64_t tmp;
> +                  memcpy(&tmp, &src[sidx].i, sizeof(tmp));
> +                  dst[didx].i = tmp;
> +                  break;
> +               }
>  	       default:
>  		  assert(!"Should not get here.");
>  		  break;
>  	       }
>  	       break;
> -
> +            case GLSL_TYPE_INT64:
> +            case GLSL_TYPE_UINT64:
> +               switch (uni->type->base_type) {
> +               case GLSL_TYPE_UINT:
> +                  *(int64_t *)&dst[didx].u = (int64_t) src[sidx].u;
> +                  break;
> +               case GLSL_TYPE_INT:
> +               case GLSL_TYPE_SAMPLER:
> +               case GLSL_TYPE_IMAGE:
> +                  *(int64_t *)&dst[didx].u = (int64_t) src[sidx].i;
> +                  break;
> +               case GLSL_TYPE_BOOL:
> +                  *(int64_t *)&dst[didx].u = src[sidx].i ? 1.0f : 0.0f;
> +                  break;
> +               case GLSL_TYPE_FLOAT:
> +                  *(int64_t *)&dst[didx].u = (int64_t) src[sidx].f;

Similarly here, the .u is really quite meaningless.

The rest looks good to me.

Cheers,
Nicolai