[Mesa-dev] [PATCH] i965: Make exec_size 16 word/byte registers use exec_size halving again.

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> Francisco Jerez <currojerez at riseup.net> writes:
>
>> Kenneth Graunke <kenneth at whitecape.org> writes:
>>
>>> On Friday, May 13, 2016 3:39:29 AM PDT Connor Abbott wrote:
>>>> My understanding is that compression isn't necessary here, at least on
>>>> newer gens (I don't know much about gen4/5). Could you explain why a
>>>> <16,16,1>:w region is illegal? It would be nice to get a PRM citation
>>>> in the comment below.
>>>
>>> Matt mentioned it was illegal in a bugzilla comment, but it certainly
>>> seems legal to me, at least with W types.  However,  we /are/ using
>>> compression...and using both together seems wrong...
>>>
>>> fs_generator::generate_code() contains:
>>>
>>>    if (dispatch_width == 16)
>>>       brw_set_default_compression_control(p, BRW_COMPRESSION_COMPRESSED);
>>>
>> That seems pretty bogus (and Connor's patch looks correct to me).  For
>> SIMD32 I had to fix the generator to set compression control to
>> BRW_COMPRESSION_COMPRESSED for instructions that write multiple
>> registers *only* -- Do you want me to try if that alone fixes the
>> regressions on 965GM?
>>
>
> I just looked at the bug report -- It looks like what is going on is
> that the instruction writes to a 32-bit 16-wide register so it must be
> compressed either way, but the code in brw_reg_from_fs_reg() fails to
> consider whether the instruction will be compressed while deciding
> whether to do the width halving or not -- AFAICT both the new and old
> code were bogus in different ways ;), we probably want something like:
>
> | if (needs_compression(inst)) {
> |    // Use half register width
> | } else {
> |    // Use half register width

Oops, of course here you'd have to use the full register width.

> | }
>
> The implementation of should_instruction_be_compressed() would have to
> be whatever we base the instruction compression control field on --
> Right now it would be something like:
>
> | static bool
> | needs_compression(fs_inst *inst)
> | {
> |    return inst->dst.component_size(inst->exec_size) > REG_SIZE;
> | }
>
> which is not quite correct where sources and destination don't agree on
> the number of components read or written, but that's already broken and
> fixing it properly probably belongs in a separate change.
>
>>> It looks like we also have code a little ways down to set compression
>>> control based on the register region, so maybe we don't the above code.
>>> But I'm kind of hesitant to delete it - we might need it for some
>>> instructions like IF which don't have sources/destinations.
>>>
>>
>> Control flow instruction have to be uncompressed anyway.  :)
>>
>>> --Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160513/6078bcc0/attachment.sig>