Matt Turner <[email protected]> writes: > On Thu, Jun 4, 2015 at 7:48 PM, Matt Turner <[email protected]> wrote: >> On Thu, Jun 4, 2015 at 9:04 AM, Francisco Jerez <[email protected]> >> 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 <[email protected]> >> >> 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 <[email protected]> >> >>> [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 <[email protected]> > >>> [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 <[email protected]> > >>> [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 <[email protected]> > >>> [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 <[email protected]> > >>> [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 <[email protected]> > >>> [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 <[email protected]> > >>> [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.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
