On Thu, Jun 4, 2015 at 7:48 PM, Matt Turner <matts...@gmail.com> wrote: > On Thu, Jun 4, 2015 at 9:04 AM, Francisco Jerez <curroje...@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 <matts...@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 <matts...@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 <matts...@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 <matts...@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 <matts...@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 <matts...@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 <matts...@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 <matts...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
