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

[Matt Turner]

On Wed, Sep 24, 2014 at 10:36 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 08/28/2014 08:10 PM, Matt Turner 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)) {
>>           compacted_count++;
>>
>>           if (INTEL_DEBUG) {
>> @@ -1130,10 +1131,7 @@ brw_compact_instructions(struct brw_compile *p, int start_offset,
>>           }
>>
>>           offset += 8;
>> -         src_offset += 16;
>>        } else {
>> -         int size = brw_inst_cmpt_control(brw, src) ? 8 : 16;
>> -
>>           /* It appears that the end of thread SEND instruction needs to be
>>            * aligned, or the GPU hangs.
>>            */
>> @@ -1154,10 +1152,9 @@ brw_compact_instructions(struct brw_compile *p, int start_offset,
>>            * place.
>>            */
>>           if (offset != src_offset) {
>> -            memmove(dst, src, size);
>> +            memmove(dst, src, sizeof(brw_inst));
>>           }
>> -         offset += size;
>> -         src_offset += size;
>> +         offset += sizeof(brw_inst);
>
> Sometimes this would increment src_offset by 8 and sometimes by 16.  Now
> src_offset is always incremented by sizeof(brw_inst).  I'm not grokking
> it enough to know if this is the same issue that Ken and Jason mentioned.

Actually, thanks for pointing that out. *That's* why the hunks that
look unrelated are in this patch -- they're actually related.

We used to generate SIMD8 instructions, compact SIMD8 instructions,
generate SIMD16 instructions, and then recompact the whole program, so
we'd pass over the previously compacted SIMD8 instructions and not
want to touch them again. I fixed this so that we only attempt to
compact the instructions we've just generated, so we should never see
a compacted instruction in the source buffer read by
brw_compact_instructions().

So that's what the assert and the removal of the
!brw_inst_cmpt_control(brw, src) check is about.

With that knowledge, we can simplify the src_offset increment, since
we know we'll never see a compacted instruction.

In any case, I've already split the patch so I'll leave it split.