Francisco Jerez <[email protected]> writes: > Kenneth Graunke <[email protected]> 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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
