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

[Matt Turner]

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.

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

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

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

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

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

> [PATCH 37/38] i965/fs: Remove dead IR construction code from the visitor.
> [PATCH 38/38] i965/fs: Drop fs_inst::force_uncompressed.