[Mesa-dev] [PATCH 00/24] i965: Refactor register classes

[Matt Turner]

On Wed, Nov 11, 2015 at 3:20 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Monday, November 02, 2015 04:29:10 PM Matt Turner wrote:
>> backend_reg (from which fs_reg, src_reg, and dst_reg inherit) includes a
>> brw_reg that's used for "hardware regs" -- precolored registers or architecture
>> registers. This leads to properties like source modifiers, the register type,
>> swizzles, and writemasks being duplicated between the derived classes and the
>> brw_reg and of course often being out of sync.
>>
>> This series removes the "fixed_hw_reg" field from backend_reg by just making
>> backend_reg inherit from brw_reg, and then removes fields duplicated in the
>> derived classes. In the process, it gets rid of HW_REG.
>>
>> This in turn simplifies a lot of code -- no longer do you have to check a
>> number of subfields if file == HW_REG.
>>
>> The last few patches begin some clean ups -- since the base of our register
>> classes is now brw_reg we don't need to do as many conversions. I've only
>> handled immediates so far and more is planned, but the series is growing large
>> and is a lot of churn already.
>>
>> The sizes of the register classes all shrink by 8 bytes:
>>
>>    backend_reg   20 -> 12
>>    fs_reg        40 -> 32
>>    src_reg       32 -> 24?
>>    dst_reg       32 -> 24?
>>
>> The remaining fields in the classes are
>>
>>    backend_reg: reg_offset
>>    fs_reg:      reladdr, subreg_offset, stride
>>    src_reg      reladdr
>>    dst_reg      reladdr
>
> Assuming you address my and Emil's feedback, the series is:
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> This is an invasive enough refactor that I believe running the
> assembly diffing tool would be worthwhile.

This is valuable, but was quite a pain. If we're looking for ideas for
useful tools, maybe this should be considered. It could be as simple
as writing the before and after assembly output to filenames based on
the name of the test execution, removing files that were identical and
leaving those with differences for manual inspection.

Anway, the differences are:

i965/vec4: Remove swizzle/writemask fields from src/dst_reg.
   We now use writemasks on null destinations (comparison instructions
with conditional mod)



Between that commit and "i965: Use BRW_MRF_COMPR4 macro in more
places.", presumably in "i965: Replace HW_REG with ARF/FIXED_GRF.", we
begin applying dependency hints on two instructions in
"bin/ext_transform_feedback-alignment" that are separated by a read of
the accumulator.



i965/fs: Replace fs_reg(imm) constructors with brw_imm_*().

Unexpected (but correct, and originally intended) type change from UD
to D in this hunk:

-   bld.AND(retype(result, BRW_REGISTER_TYPE_D), tmp, fs_reg(0xbf800000));
+   bld.AND(retype(result, BRW_REGISTER_TYPE_D), tmp, brw_imm_d(0xbf800000));



i965/vec4: Replace src_reg(imm) constructors with brw_imm_*().

Some intentional dst_null_d -> dst_null_ud changes where the sources
were UD (and one of them was being changed to brw_imm_ud).



I did notice throughout that

   bin/getteximage-formats -auto
   bin/getteximage-formats init-by-rendering -auto -fbo

have a MOV that would gain or lose a saturate modifier. I don't think
that's related to my series, but is worth investigating more (valgrind
didn't identify anything).