[Mesa-dev] [PATCH 09/13] SQUASH: i965/fs: Rework fs_visitor::lower_load_payload

[Matt Turner]

On Wed, Apr 15, 2015 at 5:13 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Wed, Apr 15, 2015 at 1:31 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Wed, Apr 1, 2015 at 6:19 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>> Instead of the complicated and broken-by-design pile of heuristics we had
>>> before, we now have a straightforward lowering:
>>>
>>>  1) All header sources are copied directly using force_writemask_all and,
>>>     since they are guaranteed to be a single register, there are no
>>>     force_sechalf issues.
>>>
>>>  2) All non-header sources are copied using the exact same force_sechalf
>>>     and saturate modifiers as the LOAD_PAYLOAD operation itself.
>>
>> Let's not do this. Nothing puts a saturate modifier on LOAD_PAYLOAD
>> today, and it is kind of confusing about what it means. Can't we have
>> fbwrites that write depth as well. I wouldn't think we wanted to
>> saturate that.
>
> Sure.  I can drop saturate and just assert that it's not set.  We do
> want to keep force_sechalf and force_writemask_all though.

I didn't think about those before, but I don't know how a load_payload
could have force_writemask_all set. Have I missed something?

I see that setup_color_payload sets force_sechalf for dual-source
fbwrites -- that's the only case we're going to have force_sechalf
set, right?

That is, the Gen < 6 case is going to be handled by passing 16-channel
sources to load_payload and letting it do compr4?

>> I don't think it buys us anything. If we just run copy propagation
>> after lower_load_payload() we'll get the code we want.
[snip]
>>> +            /* The COMPR4 code took care of the first 4 sources.  We'll let
>>> +             * the regular path handle any remaining sources.  Yes, we are
>>> +             * modifying the instruction but we're about to delete it so
>>> +             * this really doesn't hurt anything.
>>> +             */
>>> +            inst->header_size += 4;
>>
>> I mean, while the comment is a true statement, why is doing this any
>> better than just...
>>
>>> +         }
>>> +
>>> +         for (uint8_t i = inst->header_size; i < inst->sources; i++) {
>>
>> ... changing this to inst->header_size + 4?
>
> Because the inst->header_size += 4 is predicated on it being a COMPR4
> destination while the code below handles both the remaining sources
> (in the COMPR4 case) and the regular non-COMPR4 case.

Ahh, right. It'd be fewer lines (no commenting necessary) to just have
a 'start' variable that you set to header_size at the top and +=4
here.