Iago Toral <ito...@igalia.com> writes: > On Wed, 2016-09-07 at 18:48 -0700, Francisco Jerez wrote: >> This series reworks the representation of register region offsets in >> the i965 IR to be universally byte-based instead of the rather >> awkward >> split between reg_offset and subreg_offset we have in the FS back-end >> right now, or the reg_offset field currently used in the VEC4 IR >> which >> doesn't allow better granularity than 32B. The most immediate >> motivation is to enable sub-GRF offsets in the VEC4 back-end, which >> will be useful for various kinds of lowering and instruction >> splitting >> required for FP64 support on VEC4 platforms. > > Thanks a lot for taking care of this! > >> Patches 01-11 take care of scaling the regs_written and regs_read >> instruction methods on both back-ends and the reg_offset register >> field of VEC4 IR registers. The fs_reg::reg_offset and >> ::subreg_offset fields are also unified into a single register field. >> Because this part of the series is rather bulky I've tried to keep >> the >> changes as obvious and functionally equivalent as possible at the >> cost >> of introducing not particularly clever code in some cases that could >> be simplified with some knowledge of the context. Patches 31-46 make >> a second pass through the code touched in the first part of the >> series >> in order to get rid of an amount of cruft. > > I have reviewed this part and intend to continue reviewing the rest in > the following days. Might be a good idea to have another set of eyes > reviewing the series or at least skimming through it just in case. > > You might want to look at least at the second comment I made to patch 2 > and the comment to patch 7, since these might be actual problems. All > other comments are minor things or small clarifications and you can > ignore them if you want. > > With the issues I point out in patches 2 and 7 addressed (or > confirmation from your side that these are not real problems), patches > 1-11 are: > > Reviewed-by: Iago Toral Quiroga <ito...@igalia.com> > Thanks!
>> Patches 12-30 address an amount of bugs that became obvious during >> the >> conversion to byte units, some of them seem worrying enough that it >> might make sense to back-port them to stable releases. >> >> Patches 47-57 go through the VEC4 back-end and address a number of >> issues that would arise in existing optimization passes with >> non-GRF-aligned regions, which will be useful for FP64 support. It's >> likely not complete and the handling of sub-GRF offsets doesn't >> attempt to be nearly as clever as the FS back-end, but they should >> make a substantial improvement over the current situation. >> >> [PATCH 01/57] i965/fs: Replace fs_reg::reg_offset with fs_reg::offset >> expressed in bytes. >> [PATCH 02/57] i965/vec4: Replace dst/src_reg::reg_offset with >> dst/src_reg::offset expressed in bytes. >> [PATCH 03/57] i965/ir: Remove backend_reg::reg_offset. >> [PATCH 04/57] i965/fs: Replace fs_reg::subreg_offset with >> fs_reg::offset expressed in bytes. >> [PATCH 05/57] i965/fs: Add wrapper functions for fs_inst::regs_read >> and ::regs_written. >> [PATCH 06/57] i965/vec4: Add wrapper functions for >> vec4_instruction::regs_read and ::regs_written. >> [PATCH 07/57] i965/fs: Replace fs_inst::regs_written with >> ::size_written field in bytes. >> [PATCH 08/57] i965/vec4: Replace vec4_instruction::regs_written with >> ::size_written field in bytes. >> [PATCH 09/57] i965/ir: Drop backend_instruction::regs_written field. >> [PATCH 10/57] i965/fs: Replace fs_inst::regs_read with ::size_read >> using byte units. >> [PATCH 11/57] i965/vec4: Replace vec4_instruction::regs_read with >> ::size_read using byte units. >> [PATCH 12/57] i965/fs: Return more accurate read size from >> fs_inst::size_read for IMM and UNIFORM files. >> [PATCH 13/57] i965/fs: Return more accurate read size for LINTERP >> from fs_inst::size_read. >> [PATCH 14/57] i965/fs: Handle arbitrary offsets in >> brw_reg_from_fs_reg for MRF/VGRF registers. >> [PATCH 15/57] i965/fs: Handle fixed HW GRF subnr in reg_offset(). >> [PATCH 16/57] i965/fs: Take into account trailing padding in >> regs_written() and regs_read(). >> [PATCH 17/57] i965/fs: Take into account misalignment in >> regs_written() and regs_read(). >> [PATCH 18/57] i965/vec4: Take into account misalignment in >> regs_written() and regs_read(). >> [PATCH 19/57] i965/fs: Don't consider LOAD_PAYLOAD with sub-GRF >> offset to behave like a raw copy. >> [PATCH 20/57] i965/fs: Don't consider LOAD_PAYLOAD with stride > 1 >> source to behave like a raw copy. >> [PATCH 21/57] i965/fs: Compare full register offsets in cmod >> propagation pass. >> [PATCH 22/57] i965/fs: Fix can_propagate_from() source/destination >> overlap check. >> [PATCH 23/57] i965/fs: Fix LOAD_PAYLOAD handling in register coalesce >> is_nop_mov(). >> [PATCH 24/57] i965/fs: Drop fs_inst::overwrites_reg() in favor of >> regions_overlap(). >> [PATCH 25/57] i965/fs: Stop using fs_reg::in_range() in favor of >> regions_overlap(). >> [PATCH 26/57] i965/vec4: Port regions_overlap() to the vec4 IR. >> [PATCH 27/57] i965/vec4: Drop backend_reg::in_range() in favor of >> regions_overlap(). >> [PATCH 28/57] i965/fs: Take into account copy register offset during >> compute-to-mrf. >> [PATCH 29/57] i965/fs: Fix bogus sub-MRF offset calculation in >> compute-to-mrf. >> [PATCH 30/57] i965/fs: Switch mask_relative_to() used in compute-to- >> mrf to byte units. >> [PATCH 31/57] i965/fs: Fix signedness of the return value of >> fs_inst::size_read(). >> [PATCH 32/57] i965/fs: Simplify byte_offset(). >> [PATCH 33/57] i965/fs: Simplify get_fpu_lowered_simd_width() by using >> inequalities instead of rounding. >> [PATCH 34/57] i965/fs: Simplify and fix buggy stride/offset >> calculations using subscript(). >> [PATCH 35/57] i965/fs: Simplify result_live calculation in >> dead_code_eliminate(). >> [PATCH 36/57] i965/fs: Simplify a bunch of fs_inst::size_written >> calculations by using component_size(). >> [PATCH 37/57] i965/fs: Simplify copy propagation LOAD_PAYLOAD ACP >> setup. >> [PATCH 38/57] i965/fs: Change region_contained_in() to use byte >> units. >> [PATCH 39/57] i965/fs: Move region_contained_in to the IR header and >> fix for non-VGRF files. >> [PATCH 40/57] i965/fs: Use region_contained_in() in compute-to-mrf >> coalescing pass. >> [PATCH 41/57] i965/fs: Get rid of fs_inst::set_smear(). >> [PATCH 42/57] i965/fs: Misc simplification. >> [PATCH 43/57] i965/fs: Print fs_reg::offset field consistently for >> all register files. >> [PATCH 44/57] i965/vec4: Print src/dst_reg::offset field consistently >> for all register files. >> [PATCH 45/57] i965/ir: Don't print ARF subnr values twice. >> [PATCH 46/57] i965/ir: Update several stale comments. >> [PATCH 47/57] i965/vec4: Simplify src/dst_reg to brw_reg conversion >> by using byte_offset(). >> [PATCH 48/57] i965/vec4: Change opt_vector_float to keep track of the >> last offset seen in bytes. >> [PATCH 49/57] i965/vec4: Check that the write offsets match when >> setting dependency controls. >> [PATCH 50/57] i965/vec4: Compare full register offsets in >> opt_register_coalesce nop move check. >> [PATCH 51/57] i965/vec4: Don't coalesce registers with overlapping >> writes not matching the MOV source. >> [PATCH 52/57] i965/vec4: Assign correct destination offset to >> rewritten instruction in register coalesce. >> [PATCH 53/57] i965/vec4: Compare full register offsets in cmod >> propagation. >> [PATCH 54/57] i965/vec4: Fix copy propagation for non-register- >> aligned regions. >> [PATCH 55/57] i965/vec4: Don't spill non-GRF-aligned register >> regions. >> [PATCH 56/57] i965/vec4: Assert that ATTR regions are register- >> aligned. >> [PATCH 57/57] i965/vec4: Assert that pull constant load offsets are >> 16B-aligned. >>
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
