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

[Jason Ekstrand]

On Wed, Apr 15, 2015 at 6:44 PM, Matt Turner <mattst88 at gmail.com> wrote:
> 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?

No, no one (to my knowlege) sets force_writemask_all on it but I see
no reason why it shouldn't be respected.  As for saturate, we do for
fb_writes when key->clamp_fragment_color is set.
--Jason

> 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.