[Mesa-dev] [PATCH] i965/fs: Strip trailing contant zeroes in sample messages

[Neil Roberts]

Kenneth Graunke <kenneth at whitecape.org> writes:

> I like this idea!
>
> We definitely need to skip this optimization on Gen4, since the Gen4/G45
> sampler infers the texturing opcode based on the message length.  But
> for Gen5+, it should be no problem.

Ah ok, yes, I will add this.

> Matt mentioned that we have to emit zero in some cases due to hardware
> bugs.  IIRC, we used to skip some parameters in the middle - i.e. if the
> message took "u, v, r, lod"...and we were using a 2D texture...we'd omit
> 'r', since it shouldn't matter.  But it did matter - and had to be
> zeroed.  I think skipping ones at the end and reducing mlen should be
> fine.

Ok, that sounds good.

> Why not do this for all texture messages, though?  Or for that matter, all
> messages?  inst->is_tex() or inst->mlen > 0 might make sense.

Yes, I think you're right. I was just trying to be a bit conservative so
that I can test the few cases that it hits. We probably don't have many
Piglit tests that would hit this optimisation. I guess it wouldn't be
too hard to write some though.

>
>> +         fs_inst *load_payload = (fs_inst *) inst->prev;
>> +
>> +         if (load_payload->is_head_sentinel() ||
>> +             load_payload->opcode != SHADER_OPCODE_LOAD_PAYLOAD)
>> +            continue;
>> +
>> +         /* We don't want to remove the message header. Removing all of the
>> +          * parameters is avoided because it seems to cause a GPU hang but I
>> +          * can't find any documentation indicating that this is expected.
>> +          */
>> +         while (inst->mlen > inst->header_present + dispatch_width / 8 &&
>> +                load_payload->src[(inst->mlen - inst->header_present) /
>> +                                  (dispatch_width / 8) - 1].is_zero()) {
>> +            inst->mlen -= dispatch_width / 8;
>> +            progress = true;
>> +         }
>
> Another idea...you could just create a new LOAD_PAYLOAD for what you
> want, and leave the old one in place just in case it's used (with the
> assumption that it's probably not, and dead code elimination will make
> it go away).  Just a suggestion.

I'm actually modifying the sample instruction not the LOAD_PAYLOAD
instruction so it should still be in place unmodified. Unless I've
misunderstood I don't think there's a problem here with accidentally
eliminating a payload that is later used.

Thanks for the review.

Regards,
- Neil