[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:

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

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/838c9a43/attachment.sig>