[Mesa-dev] i965 FS back-end IR builder migration.

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> On Thu, Jun 4, 2015 at 7:48 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> On Thu, Jun 4, 2015 at 9:04 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> This series migrates the FS compiler back-end to the i965 IR builder I
>>> proposed a while ago as part of my ARB_shader_image_load_store series,
>>> and fixes a couple of bugs I found during the process.  It doesn't yet
>>> attempt to convert the VEC4 back-end, but the VEC4 version of the
>>> builder is also included for completeness.
>>>
>>> For a branch in testable form see:
>>> http://cgit.freedesktop.org/~currojerez/mesa/log/?h=i965-ir-builder
>>>
>>
>> I think this looks pretty good.
>>
>> Patches 1, 3-7, 9-13, 15-25, 27-30, 32-33, and 37-38 are
>>
>> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>>
>> In patches 08 and 26 -- I thought we'd decided not to do
>> force_writemask_all/force_sechalf on load_payloads, and while I think
>> that decision might not stand the test of time, your patches really
>> don't have any explanation of what's going on at all.
>>
>> There were some places that the old code did emit(BRW_OPCODE_*, ...)
>> that you converted to bld.emit(BRW_OPCODE_*, ...) and I sent comments
>> suggesting that we just go ahead and replace them with appropriate
>> MOV/AND/SHR/etc methods.
>>
>> Summary of review comments inline:
>>
>>> [PATCH 01/38] i965/fs: Introduce FS IR builder.
>>
>> A couple of unused functions should be removed. An argument named
>> 'instr' should be renamed 'inst'. With those fixed,
>>
>> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>>
>>> [PATCH 02/38] i965/vec4: Introduce VEC4 IR builder.
>>
>> I'm not sure why you're sending this? Do you actually want to commit
>> it now even without any uses?
>>
No, not necessarily.  I had made a number of changes in the VEC4 builder
since the last time I submitted it to the list, so I figured it was
worth resending just for other people to see what it looks like.

[Except for this one I think I had already answered elsewhere to all
other questions of your summary.]

>>> [PATCH 03/38] i965/fs: Allocate a common IR builder object in fs_visitor.
>>> [PATCH 04/38] i965/fs: Migrate opt_combine_constants to the IR builder.
>>> [PATCH 05/38] i965/fs: Migrate opt_peephole_predicated_break to the IR builder.
>>> [PATCH 06/38] i965/fs: Take into account all instruction fields in CSE instructions_match().
>>> [PATCH 07/38] i965/vec4: Take into account all instruction fields in CSE instructions_match().
>>
>> In the future, I'd really prefer bug fixes like these to be separate
>> from huge patch series. This has been a common feeling when reviewing
>> your series this year, and I don't know if I've previously expressed
>> it.
>>
>>> [PATCH 08/38] i965/fs: Don't drop force_writemask_all and _sechalf when copying a CSE temporary.
>>
>> Needs a commit message.
>
> With the commit message, this is
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>
>>> [PATCH 09/38] i965/fs: Migrate opt_cse to the IR builder.
>>> [PATCH 10/38] i965/fs: Create and emit instructions in one step in opt_peephole_sel.
>>> [PATCH 11/38] i965/fs: Migrate opt_peephole_sel to the IR builder.
>>> [PATCH 12/38] i965/fs: Migrate opt_sampler_eot to the IR builder.
>>> [PATCH 13/38] i965/fs: Migrate try_replace_with_sel to the IR builder.
>>> [PATCH 14/38] i965/fs: Migrate register spills and fills to the IR builder.
>>
>> Just haven't reviewed yet.
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>
>>> [PATCH 15/38] i965/fs: Migrate lower_load_payload to the IR builder.
>>> [PATCH 16/38] i965/fs: Migrate lower_integer_multiplication to the IR builder.
>>> [PATCH 17/38] i965/fs: Migrate Gen4 send dependency workarounds to the IR builder.
>>> [PATCH 18/38] i965/fs: Migrate pull constant loads to the IR builder.
>>> [PATCH 19/38] i965/fs: Migrate texturing implementation to the IR builder.
>>> [PATCH 20/38] i965/fs: Migrate untyped surface read and atomic to the IR builder.
>>> [PATCH 21/38] i965/fs: Migrate shader time to the IR builder.
>>> [PATCH 22/38] i965/fs: Migrate FS interpolation code to the IR builder.
>>> [PATCH 23/38] i965/fs: Migrate FS gl_SamplePosition/ID computation code to the IR builder.
>>> [PATCH 24/38] i965/fs: Migrate FS discard handling to the IR builder.
>>> [PATCH 25/38] i965/fs: Migrate FS alpha test to the IR builder.
>>> [PATCH 26/38] i965/fs: Migrate FS framebuffer writes to the IR builder.
>>
>> Looks like it contains a functional change hidden in a commit that
>> otherwise appears to be a refactor... all without a commit message.
>
> I'm not thrilled that we're changing the code we're emitting in this
> patch, but I'm not going to block it.
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>
>>> [PATCH 27/38] i965/fs: Migrate VS output writes to the IR builder.
>>> [PATCH 28/38] i965/fs: Migrate CS terminate message to the IR builder.
>>> [PATCH 29/38] i965/fs: Migrate NIR emit_percomp() to the IR builder.
>>> [PATCH 30/38] i965/fs: Migrate NIR variable handling to the IR builder.
>>> [PATCH 31/38] i965/fs: Migrate translation of NIR control flow to the IR builder.
>>
>> Sent a question.
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>
>>> [PATCH 32/38] i965/fs: Migrate translation of NIR ALU instructions to the IR builder.
>>> [PATCH 33/38] i965/fs: Migrate translation of NIR intrinsics to the IR builder.
>>> [PATCH 34/38] i965/fs: Migrate translation of NIR texturing instructions to the IR builder.
>>
>> I'd like to pull out the removal of the this->base_ir assignments into
>> a separate patch.
>
> With the base_ir assignments moved to patch 37, this is
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>
>>> [PATCH 35/38] i965/fs: Migrate test_fs_saturate_propagation to the IR builder.
>>> [PATCH 36/38] i965/fs: Migrate test_fs_cmod_propagation to the IR builder.
>>
>> I think these two should get the full builder treatment -- use the
>> actual instruction methods, set_condmod, set_predicate, etc.
>
> The second versions of these are
>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
>
>>> [PATCH 37/38] i965/fs: Remove dead IR construction code from the visitor.
>>> [PATCH 38/38] i965/fs: Drop fs_inst::force_uncompressed.
>
>
>
> I think everything except 02/38 is reviewed. Let me know if that's not the case.

Yeah, you've reviewed everything that needed review already.

Thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150608/5200f5a8/attachment.sig>