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

[Matt Turner]

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?
>
>> [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.