[Mesa-dev] [PATCH 00/24] i965: Refactor register classes
backend_reg (from which fs_reg, src_reg, and dst_reg inherit) includes a brw_reg that's used for "hardware regs" -- precolored registers or architecture registers. This leads to properties like source modifiers, the register type, swizzles, and writemasks being duplicated between the derived classes and the brw_reg and of course often being out of sync. This series removes the "fixed_hw_reg" field from backend_reg by just making backend_reg inherit from brw_reg, and then removes fields duplicated in the derived classes. In the process, it gets rid of HW_REG. This in turn simplifies a lot of code -- no longer do you have to check a number of subfields if file == HW_REG. The last few patches begin some clean ups -- since the base of our register classes is now brw_reg we don't need to do as many conversions. I've only handled immediates so far and more is planned, but the series is growing large and is a lot of churn already. The sizes of the register classes all shrink by 8 bytes: backend_reg 20 -> 12 fs_reg40 -> 32 src_reg 32 -> 24? dst_reg 32 -> 24? The remaining fields in the classes are backend_reg: reg_offset fs_reg: reladdr, subreg_offset, stride src_reg reladdr dst_reg reladdr ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/24] i965: Refactor register classes
Hi Matt, On 3 November 2015 at 00:29, Matt Turner wrote: > backend_reg (from which fs_reg, src_reg, and dst_reg inherit) includes a > brw_reg that's used for "hardware regs" -- precolored registers or > architecture > registers. This leads to properties like source modifiers, the register type, > swizzles, and writemasks being duplicated between the derived classes and the > brw_reg and of course often being out of sync. > Great work ! While looking around the *_reg::init() I noticed a few places where we don't derive (clone all the member variables) new *_reg correctly. Plus I believe I've seen a few initialised member variables - mostly on the abs/negate front. I'm curious of there could attribute to the intermittent piglit failures that people occasionally see. > This series removes the "fixed_hw_reg" field from backend_reg by just making > backend_reg inherit from brw_reg, and then removes fields duplicated in the > derived classes. In the process, it gets rid of HW_REG. > I remember initially looking at fixed_hw_reg and having a few 'wtf' moments. Inheriting from brw_reg will certainly help other (fs/vec4 IR) new-comers. > This in turn simplifies a lot of code -- no longer do you have to check a > number of subfields if file == HW_REG. > > The last few patches begin some clean ups -- since the base of our register > classes is now brw_reg we don't need to do as many conversions. I've only > handled immediates so far and more is planned, but the series is growing large > and is a lot of churn already. > > The sizes of the register classes all shrink by 8 bytes: > >backend_reg 20 -> 12 >fs_reg40 -> 32 >src_reg 32 -> 24? >dst_reg 32 -> 24? > > The remaining fields in the classes are > >backend_reg: reg_offset >fs_reg: reladdr, subreg_offset, stride >src_reg reladdr >dst_reg reladdr > I'm thinking that we can also move reladdr to backend_reg. Although that'll be obviously follow up patches. Fwiw I really like the whole series and barring a few small comments I would love to see it land. Big thanks for doing this. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/24] i965: Refactor register classes
Hi Matt, A short summary [PATCH 06/24] i965: Add and use enum brw_reg_file. s/GRF/BRW_ARCHITECTURE_REGISTER_FILE/ - already addressed in your branch. [PATCH 12/24] i965: Initialize registers' file to BAD_FILE. With the fs_visitor::init() hunk dropped and memset(this->outputs, 0...) removed. [PATCH 18/24] i965: Combine register file field. Please squash the two fixup patches that I've sent. [PATCH 20/24] Revert "i965: Have brw_imm_vf4() take the vector components as integer values." Regardless if VF_ZERO/ONE or brw_float_to_vf() is used. [PATCH 23/24] i965/vec4: Replace src_reg(imm) constructors with brw_imm_*(). Keep the memcpy(), although we really should address the uninitialised imm[] warning/bug. In a separate commit of course and optionally nuke the memcpy. With these and a jenkins spin to catch any subtle bugs the series is Reviewed-by: Emil Velikov Thanks again for reworking/clearing these up. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/24] i965: Refactor register classes
On Monday, November 02, 2015 04:29:10 PM Matt Turner wrote: > backend_reg (from which fs_reg, src_reg, and dst_reg inherit) includes a > brw_reg that's used for "hardware regs" -- precolored registers or > architecture > registers. This leads to properties like source modifiers, the register type, > swizzles, and writemasks being duplicated between the derived classes and the > brw_reg and of course often being out of sync. > > This series removes the "fixed_hw_reg" field from backend_reg by just making > backend_reg inherit from brw_reg, and then removes fields duplicated in the > derived classes. In the process, it gets rid of HW_REG. > > This in turn simplifies a lot of code -- no longer do you have to check a > number of subfields if file == HW_REG. > > The last few patches begin some clean ups -- since the base of our register > classes is now brw_reg we don't need to do as many conversions. I've only > handled immediates so far and more is planned, but the series is growing large > and is a lot of churn already. > > The sizes of the register classes all shrink by 8 bytes: > >backend_reg 20 -> 12 >fs_reg40 -> 32 >src_reg 32 -> 24? >dst_reg 32 -> 24? > > The remaining fields in the classes are > >backend_reg: reg_offset >fs_reg: reladdr, subreg_offset, stride >src_reg reladdr >dst_reg reladdr Assuming you address my and Emil's feedback, the series is: Reviewed-by: Kenneth Graunke This is an invasive enough refactor that I believe running the assembly diffing tool would be worthwhile. Nice work :) I like the result. signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/24] i965: Refactor register classes
On Wed, Nov 11, 2015 at 3:20 PM, Kenneth Graunke wrote: > On Monday, November 02, 2015 04:29:10 PM Matt Turner wrote: >> backend_reg (from which fs_reg, src_reg, and dst_reg inherit) includes a >> brw_reg that's used for "hardware regs" -- precolored registers or >> architecture >> registers. This leads to properties like source modifiers, the register type, >> swizzles, and writemasks being duplicated between the derived classes and the >> brw_reg and of course often being out of sync. >> >> This series removes the "fixed_hw_reg" field from backend_reg by just making >> backend_reg inherit from brw_reg, and then removes fields duplicated in the >> derived classes. In the process, it gets rid of HW_REG. >> >> This in turn simplifies a lot of code -- no longer do you have to check a >> number of subfields if file == HW_REG. >> >> The last few patches begin some clean ups -- since the base of our register >> classes is now brw_reg we don't need to do as many conversions. I've only >> handled immediates so far and more is planned, but the series is growing >> large >> and is a lot of churn already. >> >> The sizes of the register classes all shrink by 8 bytes: >> >>backend_reg 20 -> 12 >>fs_reg40 -> 32 >>src_reg 32 -> 24? >>dst_reg 32 -> 24? >> >> The remaining fields in the classes are >> >>backend_reg: reg_offset >>fs_reg: reladdr, subreg_offset, stride >>src_reg reladdr >>dst_reg reladdr > > Assuming you address my and Emil's feedback, the series is: > > Reviewed-by: Kenneth Graunke > > This is an invasive enough refactor that I believe running the > assembly diffing tool would be worthwhile. This is valuable, but was quite a pain. If we're looking for ideas for useful tools, maybe this should be considered. It could be as simple as writing the before and after assembly output to filenames based on the name of the test execution, removing files that were identical and leaving those with differences for manual inspection. Anway, the differences are: i965/vec4: Remove swizzle/writemask fields from src/dst_reg. We now use writemasks on null destinations (comparison instructions with conditional mod) Between that commit and "i965: Use BRW_MRF_COMPR4 macro in more places.", presumably in "i965: Replace HW_REG with ARF/FIXED_GRF.", we begin applying dependency hints on two instructions in "bin/ext_transform_feedback-alignment" that are separated by a read of the accumulator. i965/fs: Replace fs_reg(imm) constructors with brw_imm_*(). Unexpected (but correct, and originally intended) type change from UD to D in this hunk: - bld.AND(retype(result, BRW_REGISTER_TYPE_D), tmp, fs_reg(0xbf80)); + bld.AND(retype(result, BRW_REGISTER_TYPE_D), tmp, brw_imm_d(0xbf80)); i965/vec4: Replace src_reg(imm) constructors with brw_imm_*(). Some intentional dst_null_d -> dst_null_ud changes where the sources were UD (and one of them was being changed to brw_imm_ud). I did notice throughout that bin/getteximage-formats -auto bin/getteximage-formats init-by-rendering -auto -fbo have a MOV that would gain or lose a saturate modifier. I don't think that's related to my series, but is worth investigating more (valgrind didn't identify anything). ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
