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

[Ian Romanick]

On 10/27/2016 06:02 AM, Nicolai Hähnle wrote:
> 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;?

I think we need to do the memcpy (and should below too) to avoid
strict-aliasing problems.  I don't recall all the details.  Perhaps Matt
can offer some guidance.

>> +               }
>>             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
>