[Mesa-dev] [PATCH 11/14] i965/compaction: Add support for G45.

[Matt Turner]

On Tue, Sep 23, 2014 at 2:08 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Tue, Sep 23, 2014 at 1:28 PM, Matt Turner <mattst88 at gmail.com> wrote:
>>
>> On Tue, Sep 23, 2014 at 1:19 PM, Jason Ekstrand <jason at jlekstrand.net>
>> wrote:
>> > On Thu, Aug 28, 2014 at 8:10 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> >>     int offset = 0;
>> >> @@ -1319,17 +1323,22 @@ brw_compact_instructions(struct brw_compile *p,
>> >> int start_offset,
>> >>           offset += sizeof(brw_compact_inst);
>> >>        } else {
>> >>           /* It appears that the end of thread SEND instruction needs
>> >> to
>> >> be
>> >> -          * aligned, or the GPU hangs.
>> >> +          * aligned, or the GPU hangs. All uncompacted instructions
>> >> need
>> >> to be
>> >> +          * aligned on G45.
>> >>            */
>> >> -         if ((brw_inst_opcode(brw, src) == BRW_OPCODE_SEND ||
>> >> -              brw_inst_opcode(brw, src) == BRW_OPCODE_SENDC) &&
>> >> -             brw_inst_eot(brw, src) &&
>> >> -             (offset & sizeof(brw_compact_inst)) != 0) {
>> >> +         if ((offset & sizeof(brw_compact_inst)) != 0 &&
>> >> +             (((brw_inst_opcode(brw, src) == BRW_OPCODE_SEND ||
>> >> +                brw_inst_opcode(brw, src) == BRW_OPCODE_SENDC) &&
>> >> +               brw_inst_eot(brw, src)) ||
>> >> +              brw->is_g4x)) {
>> >>              brw_compact_inst *align = store + offset;
>> >>              memset(align, 0, sizeof(*align));
>> >> -            brw_compact_inst_set_opcode(align, BRW_OPCODE_NOP);
>> >> +            brw_compact_inst_set_opcode(align, brw->is_g4x ?
>> >> BRW_OPCODE_NENOP :
>> >> +
>> >> BRW_OPCODE_NOP);
>> >>              brw_compact_inst_set_cmpt_control(align, true);
>> >>              offset += sizeof(brw_compact_inst);
>> >> +            compacted_count--;
>> >> +            compacted_counts[src_offset / sizeof(brw_inst)] =
>> >> compacted_count;
>> >
>> >
>> > Do these two lines really belong in this patch?  They seem completely
>> > unrelated to stuff on G45.
>>
>> Yes, because if you have to insert a padding NENOP, you need to update
>> the compaction_count (which is really a metric of how far offset you
>> are because of the space saved by compacted instructions).
>>
>> So, really what it's doing is not considering compacted instructions
>> as saving space if we had to insert a padding NENOP after it.
>
>
> Why didn't we need that post-gen5 and, if we didn't, why are we doing it
> unconditionally?

We don't need it post-Gen5 because those platforms jump instructions
can jump to 64-bit aligned instructions, while Gen4 can't.

It's not unconditional -- it happens only when we have to insert a
padding instruction:

> +         if ((offset & sizeof(brw_compact_inst)) != 0 &&
> +             (((brw_inst_opcode(brw, src) == BRW_OPCODE_SEND ||
> +                brw_inst_opcode(brw, src) == BRW_OPCODE_SENDC) &&
> +               brw_inst_eot(brw, src)) ||
> +              brw->is_g4x)) {