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

[Ian Romanick]

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.

It also keeps you from accidentally modifying it when you didn't mean 
to.  I have to side with John Carmack on this one:

     "I am a full const nazi nowadays, and I chide any programmer
     that doesn’t const every variable and parameter that can be."

If you need to make the constant a different size for a different 
platform, use the ?: operator.

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev