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

[Eric Anholt]

Mark Mueller <markkmueller at gmail.com> writes:

> 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.

If your compiler isn't doing that, it's a problem with your compiler,
not the code being compiled, and you need to fix that in your build
environment.

> 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 */

The const keyword doesn't tell the compiler anyhing except "keep the
developer from trying to modify this", which just makes things
irritating when somebody comes along later to add something like "oh,
and on gen11 we need to reserve an extra 4k" or whatever.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130809/7b6943d5/attachment.pgp>