[Mesa-dev] [PATCH 10/10] mesa: Remove unneeded bitfield widths from the VAO.

[Mathias Fröhlich]

Hi,

On Tuesday, 20 November 2018 09:23:53 CET Timothy Arceri wrote:
> Just curious. Why is this change better? Why not just leave these as is 
> if it's not hurting anything?

Well, you mean the unsigned char -> GLubyte?
That very similar part Brian requested with patch #9. So I assume this is what he intents also.
And yes there tends to be the rule that if its api visible use a gl type, if not use plain c types.
Sometimes I got review requests in the past about the opposite ...
I personally don't care and there is not much reason to care.
Finally some of you seem to like plain c types and some others don't.
Brian usually tends to GL types ...
Intel people more often request plain c types ...
... is what experience tells.
... usually this is what I anticipate, this time I did not, so I got the change request :-).

For dropping the bitfield widths?
If this is not gaining some space to have them, I tend not to use bitfields.
When using bitfields the compiler has to mask out other bits on read an
may be load the previous content on write. So all together more instructions
and more traffic on the cache at least. These extra instructions may not take
significant time, but why do that extra work if not needed and beneficial?
If we need an extra bit in that struct in the future, we can reintroduce bitfields again.

So, finally bitfields potentially do hurt IMO a small bit. But they may pay off by either resolving
memory pressure issues, which is something you cant really trade against potential
speed gains as the weighting depends on your problem. Or they pay by a smaller
cache footprint finally. Alignment improvements may be...
That is all nothing guaranteed, but again my thought behind is that I cant see to
further reduce the effective struct size of gl_array_attributes in a way that putting that into
an array like it is used in the VAO does not again blow up due to alignment requirements
of the pointer stored in there. So, why mess with the potential hit of a bitfield where there
is no gain by using them ...

Does that explain?

best

Mathias

> 
> On 20/11/18 6:24 pm, Mathias.Froehlich at gmx.net wrote:
> > From: Mathias Fröhlich <mathias.froehlich at web.de>
> > 
> > With the current VAO layout we do not need to make these
> > fields a bitfield. We get a tight struct layout with this change
> > for VAO attributes.
> > 
> > v2: Change unsigned char -> GLubyte.
> > 
> > Reviewed-by: Brian Paul <brianp at vmware.com>
> > Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> > ---
> >   src/mesa/main/mtypes.h | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 62d3b75a36..157d45bc0b 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -1439,7 +1439,7 @@ struct gl_array_attributes
> >      /** Stride as specified with gl*Pointer() */
> >      GLshort Stride;
> >      /** Index into gl_vertex_array_object::BufferBinding[] array */
> > -   unsigned BufferBindingIndex:6;
> > +   GLubyte BufferBindingIndex;
> >   
> >      /**
> >       * Derived effective buffer binding index
> > @@ -1454,7 +1454,7 @@ struct gl_array_attributes
> >       * Note that _mesa_update_vao_derived_arrays is called when binding
> >       * the VAO to Array._DrawVAO.
> >       */
> > -   unsigned _EffBufferBindingIndex:6;
> > +   GLubyte _EffBufferBindingIndex;
> >      /**
> >       * Derived effective relative offset.
> >       *
> > 
>