[Mesa-dev] [i965][V2] i965/draw: Move constant formation outside of for loop and use an enum.

[Mark Mueller]

On Thu, Aug 8, 2013 at 2:19 PM, Eric Anholt <eric at anholt.net> wrote:

> Mark Mueller <markkmueller at gmail.com> writes:
> > Signed-off-by: Mark Mueller <MarkKMueller at gmail.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_draw.c | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> > index 6170d07..1b5ed55 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -367,6 +367,12 @@ static bool brw_try_draw_prims( struct gl_context
> *ctx,
> >     bool retval = true;
> >     GLuint i;
> >     bool fail_next = false;
> > +   const int estimated_max_prim_size =
> > +           512 + /* batchbuffer commands */
> > +           ((BRW_MAX_TEX_UNIT * (sizeof(struct brw_sampler_state) +
> sizeof(struct gen5_sampler_default_color)))) +
> > +           1024 + /* gen6 VS push constants */
> > +           1024 + /* gen6 WM push constants */
> > +           512; /* misc. pad */
>
> What's the point of this change?  Moving loop invariants out of loops is
> something basic that your compiler does,


Is that universally true for the code as it looked originally (see below)?
I've worked on embedded Atom and other systems with heavily dumbed down gcc
or other cross compilers. For instance there is a good chance that the
compilers from vehicle infotainment systems that I've worked on recently
would generate assembly for each line of code below inside the loop.

original code:

      int estimated_max_prim_size;


      estimated_max_prim_size = 512; /* batchbuffer commands */
      estimated_max_prim_size += (BRW_MAX_TEX_UNIT *
				  (sizeof(struct brw_sampler_state) +
				   sizeof(struct gen5_sampler_default_color)));
      estimated_max_prim_size += 1024; /* gen6 VS push constants */
      estimated_max_prim_size += 1024; /* gen6 WM push constants */
      estimated_max_prim_size += 512; /* misc. pad */
.

.



> and you're moving the
> definition farther from its use.
>

Sure, yet it's in the company of fail_next with a similar problem. What
about keeping the definition inside the for loop but adding the const
keyword and adding all of the immediates as one operation?

   for (i = 0; i < nr_prims; i++) {

         const int estimated_max_prim_size =
                 512 + /* batchbuffer commands */
                 ((BRW_MAX_TEX_UNIT * (sizeof(struct brw_sampler_state) +
sizeof(struct gen5_sampler_default_color)))) +
                 1024 + /* gen6 VS push constants */
                 1024 + /* gen6 WM push constants */
                 512; /* misc. pad */

I'm also fine with dropping the whole thing and moving on as the real point
of this was to calibrate myself to the contribution process. It's been good
for that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130808/414d5ca7/attachment-0001.html>