[Mesa-dev] [PATCH 05/14] i965/compaction: Make src_offset local to the for loop.

[Matt Turner]

On Tue, Sep 23, 2014 at 11:49 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>
>
> On Thu, Aug 28, 2014 at 8:10 PM, Matt Turner <mattst88 at gmail.com> wrote:
>>
>> ---
>>  src/mesa/drivers/dri/i965/brw_eu_compact.c | 17 +++++++----------
>>  1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> b/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> index 5617947..dd32175 100644
>> --- a/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> +++ b/src/mesa/drivers/dri/i965/brw_eu_compact.c
>> @@ -737,6 +737,8 @@ brw_try_compact_instruction(struct brw_context *brw,
>> brw_compact_inst *dst,
>>  {
>>     brw_compact_inst temp;
>>
>> +   assert(brw_inst_cmpt_control(brw, src) == 0);
>> +
>>     if (brw_inst_opcode(brw, src) == BRW_OPCODE_IF ||
>>         brw_inst_opcode(brw, src) == BRW_OPCODE_ELSE ||
>>         brw_inst_opcode(brw, src) == BRW_OPCODE_ENDIF ||
>> @@ -1105,10 +1107,10 @@ brw_compact_instructions(struct brw_compile *p,
>> int start_offset,
>>     if (brw->gen < 6)
>>        return;
>>
>> -   int src_offset;
>>     int offset = 0;
>>     int compacted_count = 0;
>> -   for (src_offset = 0; src_offset < p->next_insn_offset - start_offset;)
>> {
>> +   for (int src_offset = 0; src_offset < p->next_insn_offset -
>> start_offset;
>> +        src_offset += sizeof(brw_inst)) {
>>        brw_inst *src = store + src_offset;
>>        void *dst = store + offset;
>>
>> @@ -1117,8 +1119,7 @@ brw_compact_instructions(struct brw_compile *p, int
>> start_offset,
>>
>>        brw_inst saved = *src;
>>
>> -      if (!brw_inst_cmpt_control(brw, src) &&
>> -          brw_try_compact_instruction(brw, dst, src)) {
>> +      if (brw_try_compact_instruction(brw, dst, src)) {
>
>
> You're now ignoring the compaction control bit.  Is that intentional?  If
> so, then this looks ok, but it is a functional change and the commit message
> should say more than rescoping a variable.

Right, this was Ken's feedback as well. The conclusion was that this
was an intentional change, but that it was intended to be in a
separate patch. I'm going to split it out.