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

[Kenneth Graunke]

On 08/09/2013 12:33 AM, Eric Anholt wrote:
> 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.

Just a friendly reminder that this number is kind of bullshit anyway:

1. 512 bytes for batchbuffer commands?  On what generation?
2. Surface states anybody?  That's potentially 1-3k of space not tracked
3. With hardware contexts, we don't always emit the full state anyway...
4. BLORP shares batches with normal drawing now...

As far as I know, this is only used to flush the batch when it's 
approaching full.  If we filled up the last little bit, and then ran out 
of space, we'd have to start over, wasting CPU time.

If we want per-generation numbers, I think the right solution is to do:

static const int estimated_max_prim_size[] = { ... }
... estimated_max_prim_size[brw->gen - 4] ...

or a helper function, not if-ladders which +=.

But this is all pretty dodgy anyway.

--Ken