[Mesa-dev] [PATCH 13/20] i965: Use basic-block aware insertion/removal functions.

[Matt Turner]

On Thu, Aug 7, 2014 at 7:55 AM, Pohjolainen, Topi
<topi.pohjolainen at intel.com> wrote:
> On Thu, Jul 24, 2014 at 07:54:20PM -0700, Matt Turner wrote:
>> To avoid invalidating and recreating the control flow graph. Also stop
>> invalidating the CFG in places we didn't add or remove an instruction.
>>
>> cfg calculations:     202951 -> 80307 (-60.43%)
>> ---
[snip]
>> -   invalidate_live_intervals();
>> +   invalidate_live_intervals(false);
>>     calculate_live_intervals();
>
> With a quick glance this looked a little odd, we don't invalidate but
> calculate nevertheless. But we actually do invalidate, the intervals, but
> not the cfg. One just needs to remember what the argument actually stands for.

Right, I'm looking forward to deleting the argument from
invalidate_live_intervals(). I hate boolean arguments like this.

>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> index 390448a..ebee42f 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> @@ -403,7 +403,7 @@ vec4_visitor::opt_copy_propagation()
>>     }
>>
>>     if (progress)
>> -      invalidate_live_intervals();
>> +      invalidate_live_intervals(false);
>
> The patch either explicitly changes an iteration to use blocks and block-aware
> manipulation routines, or relies on the fact that the iteration already
> operates with blocks.
> This is the only occurence that initially looked to be using the flat
> instruction list (even though the comment in the existing code says that it
> operates on basic blocks). Closer inspection reveals that it actually doesn't
> add or remove instructions, it just modifies existing, right?

Indeed, exactly right.

> Somebody else should have a look as well, but
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
>
> And just as a thought, splitting this to three parts might assist bisecting
> later on. One patch just changing the cases where iteration already works
> with blocks, another dealing with those cases where the iteration needs to be
> modified as well and one patch dealing with the case here.

Yes, probably a good idea.