| Message ID | 20180503171703.GA4233@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | PowerPC address support clean, patch 1 of 4 | expand |
These patches were previously posted in March as a RFC, and I would like to check them into the trunk. These patches make the mode_supports* functions use similar names for the functions that return if a mode supports D-FORM, DS-FORM, and/or DQ-FORM instructions, and add the ability to ask whether a particular reload register class supports a particular D*-form instruction. This is patch #3 of 4. I have done a bootstrap with patches 1-4 and did a make check comparison on a little endian power8 system, and there were no regressions. Can I check it in? 2018-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> * config/rs6000/rs6000.c (mode_supports_d_form): Rename mode_supports_vmx_dform to mode_supports_d_form. Add an optional argument to say which reload register class to use. Change all callers to pass in the RELOAD_REG_VMX class explicitly. (rs6000_secondary_reload): Likewise. (rs6000_preferred_reload_class): Likewise. (rs6000_secondary_reload_class): Likewise.
These patches were previously posted in March as a RFC, and I would like to check them into the trunk. These patches make the mode_supports* functions use similar names for the functions that return if a mode supports D-FORM, DS-FORM, and/or DQ-FORM instructions, and add the ability to ask whether a particular reload register class supports a particular D*-form instruction. This is patch #4 of 4. I have done a bootstrap with patches 1-4 and did a make check comparison on a little endian power8 system, and there were no regressions. Can I check it in? 2018-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> * config/rs6000/rs6000.c (mode_supports_pre_incdec_p): Add additional argument to specify the reload register class to use, defaulting to RELOAD_REG_ANY. (mode_supports_pre_modify_p): Likewise. (mode_supports_dq_form): Likewise.
Hi Mike, On Thu, May 03, 2018 at 01:17:03PM -0400, Michael Meissner wrote: > 2018-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > * config/rs6000/rs6000.c (mode_supports_dq_form): Rename > mode_supports_vsx_dform_quad to mode_supports_dq_form. > (mode_supports_vsx_dform_quad): Likewise. > (quad_address_p): Likewise. > (reg_offset_addressing_ok_p): Likewise. > (offsettable_ok_by_alignment): Likewise. > (rs6000_legitimate_offset_address_p): Likewise. > (legitimate_lo_sum_address_p): Likewise. > (rs6000_legitimize_address): Likewise. > (rs6000_legitimize_reload_address): Likewise. > (rs6000_secondary_reload_inner): Likewise. > (rs6000_preferred_reload_class): Likewise. > (rs6000_output_move_128bit): Likewise. * config/rs6000/rs6000.c (mode_supports_vsx_dform_quad): Rename to ... (mode_supports_dq_form): ... this. Update all callers. > --- gcc/config/rs6000/rs6000.c (revision 259864) > +++ gcc/config/rs6000/rs6000.c (working copy) > @@ -649,7 +649,7 @@ mode_supports_vmx_dform (machine_mode mo > is more limited than normal d-form addressing in that the offset must be > aligned on a 16-byte boundary. */ > static inline bool > -mode_supports_vsx_dform_quad (machine_mode mode) > +mode_supports_dq_form (machine_mode mode) > { > return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_QUAD_OFFSET) > != 0); Will this eventually handle all DQ-form, not just vector? Is it supposed to? Okay for trunk with the changelog fixed. Thanks! Segher
On Thu, May 03, 2018 at 01:22:10PM -0400, Michael Meissner wrote: > 2018-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > * config/rs6000/rs6000.c (mode_supports_d_form): Rename > mode_supports_vmx_dform to mode_supports_d_form. Add an optional > argument to say which reload register class to use. Change all > callers to pass in the RELOAD_REG_VMX class explicitly. > (rs6000_secondary_reload): Likewise. > (rs6000_preferred_reload_class): Likewise. > (rs6000_secondary_reload_class): Likewise. Please don't say "likewise" unless the change is actually similar. > -/* Return true if we have D-form addressing in altivec registers. */ > +/* Return true if we have D-form addressing (register+offset) in either a > + specific reload register class or whether some reload register class > + supports d-form addressing. */ > static inline bool > -mode_supports_vmx_dform (machine_mode mode) > +mode_supports_d_form (machine_mode mode, > + enum rs6000_reload_reg_type rt = RELOAD_REG_ANY) > { > - return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0); > + return ((reg_addr[mode].addr_mask[rt] & RELOAD_REG_OFFSET) != 0); > } Will this overload help anything? It does not look that way, all current callers use a different argument (and all the same). Overloads are nice if they make things *easier* for the reader, not harder. Same as with all other syntactic sugar. Segher
On Wed, May 09, 2018 at 06:14:27PM -0500, Segher Boessenkool wrote: > On Thu, May 03, 2018 at 01:22:10PM -0400, Michael Meissner wrote: > > 2018-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > > > * config/rs6000/rs6000.c (mode_supports_d_form): Rename > > mode_supports_vmx_dform to mode_supports_d_form. Add an optional > > argument to say which reload register class to use. Change all > > callers to pass in the RELOAD_REG_VMX class explicitly. > > (rs6000_secondary_reload): Likewise. > > (rs6000_preferred_reload_class): Likewise. > > (rs6000_secondary_reload_class): Likewise. > > Please don't say "likewise" unless the change is actually similar. > > > -/* Return true if we have D-form addressing in altivec registers. */ > > +/* Return true if we have D-form addressing (register+offset) in either a > > + specific reload register class or whether some reload register class > > + supports d-form addressing. */ > > static inline bool > > -mode_supports_vmx_dform (machine_mode mode) > > +mode_supports_d_form (machine_mode mode, > > + enum rs6000_reload_reg_type rt = RELOAD_REG_ANY) > > { > > - return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0); > > + return ((reg_addr[mode].addr_mask[rt] & RELOAD_REG_OFFSET) != 0); > > } > > Will this overload help anything? It does not look that way, all current > callers use a different argument (and all the same). All current callers just use the ANY option (except for these calls). However in the future, I'm planning on calling these functions with the specific reload register class (hence the change). > Overloads are nice if they make things *easier* for the reader, not harder. > Same as with all other syntactic sugar.
Hi, On Thu, May 03, 2018 at 01:23:24PM -0400, Michael Meissner wrote: > -/* Helper function to say whether a mode supports PRE_INC or PRE_DEC. */ > +/* Helper function to say whether a mode supports PRE_INC or PRE_DEC in a given > + reload register class or if some reload register class supports it. */ > static inline bool > -mode_supports_pre_incdec_p (machine_mode mode) > +mode_supports_pre_incdec_p (machine_mode mode, > + enum rs6000_reload_reg_type rt = RELOAD_REG_ANY) > { > - return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_INCDEC) > - != 0); > + return ((reg_addr[mode].addr_mask[rt] & RELOAD_REG_PRE_INCDEC) != 0); > } Same issue here: does the default argument help, or hurt? The function names now do not describe what the function does, either :-/ Segher
On Wed, May 09, 2018 at 05:54:53PM -0500, Segher Boessenkool wrote: > Hi Mike, > > On Thu, May 03, 2018 at 01:17:03PM -0400, Michael Meissner wrote: > > 2018-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > > > * config/rs6000/rs6000.c (mode_supports_dq_form): Rename > > mode_supports_vsx_dform_quad to mode_supports_dq_form. > > (mode_supports_vsx_dform_quad): Likewise. > > (quad_address_p): Likewise. > > (reg_offset_addressing_ok_p): Likewise. > > (offsettable_ok_by_alignment): Likewise. > > (rs6000_legitimate_offset_address_p): Likewise. > > (legitimate_lo_sum_address_p): Likewise. > > (rs6000_legitimize_address): Likewise. > > (rs6000_legitimize_reload_address): Likewise. > > (rs6000_secondary_reload_inner): Likewise. > > (rs6000_preferred_reload_class): Likewise. > > (rs6000_output_move_128bit): Likewise. > > * config/rs6000/rs6000.c (mode_supports_vsx_dform_quad): Rename to ... > (mode_supports_dq_form): ... this. Update all callers. > > > > --- gcc/config/rs6000/rs6000.c (revision 259864) > > +++ gcc/config/rs6000/rs6000.c (working copy) > > @@ -649,7 +649,7 @@ mode_supports_vmx_dform (machine_mode mo > > is more limited than normal d-form addressing in that the offset must be > > aligned on a 16-byte boundary. */ > > static inline bool > > -mode_supports_vsx_dform_quad (machine_mode mode) > > +mode_supports_dq_form (machine_mode mode) > > { > > return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_QUAD_OFFSET) > > != 0); > > Will this eventually handle all DQ-form, not just vector? Is it supposed > to? Other than LQ (and LTPTR which GCC doesn't generate), all dq-form instructions load/store VSX registers. The problem is GCC will not generate LQ on little endian systems because the registers are loaded in a big endian fashion. It was more given we now have mode_supports_d_form and mode_supports_ds_form, I was just making the name similar.
On Thu, May 10, 2018 at 04:55:02PM -0500, Segher Boessenkool wrote: > Hi, > > On Thu, May 03, 2018 at 01:23:24PM -0400, Michael Meissner wrote: > > -/* Helper function to say whether a mode supports PRE_INC or PRE_DEC. */ > > +/* Helper function to say whether a mode supports PRE_INC or PRE_DEC in a given > > + reload register class or if some reload register class supports it. */ > > static inline bool > > -mode_supports_pre_incdec_p (machine_mode mode) > > +mode_supports_pre_incdec_p (machine_mode mode, > > + enum rs6000_reload_reg_type rt = RELOAD_REG_ANY) > > { > > - return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_INCDEC) > > - != 0); > > + return ((reg_addr[mode].addr_mask[rt] & RELOAD_REG_PRE_INCDEC) != 0); > > } > > Same issue here: does the default argument help, or hurt? The function > names now do not describe what the function does, either :-/ I dunno, to me it describes it because each of the 3 reload register classes can have different constraints. So when you are in secondary reload and after wards, you will want the specific register class. Before register allocation, you just want to know if any register class supports the access type for the mode.
On Thu, May 10, 2018 at 05:49:12PM -0400, Michael Meissner wrote: > > > -/* Return true if we have D-form addressing in altivec registers. */ > > > +/* Return true if we have D-form addressing (register+offset) in either a > > > + specific reload register class or whether some reload register class > > > + supports d-form addressing. */ > > > static inline bool > > > -mode_supports_vmx_dform (machine_mode mode) > > > +mode_supports_d_form (machine_mode mode, > > > + enum rs6000_reload_reg_type rt = RELOAD_REG_ANY) > > > { > > > - return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0); > > > + return ((reg_addr[mode].addr_mask[rt] & RELOAD_REG_OFFSET) != 0); > > > } > > > > Will this overload help anything? It does not look that way, all current > > callers use a different argument (and all the same). > > All current callers just use the ANY option (except for these calls). However > in the future, I'm planning on calling these functions with the specific reload > register class (hence the change). No, they use RELOAD_REG_VMX. Unless there is something extra tricky about your patch? > > Overloads are nice if they make things *easier* for the reader, not harder. > > Same as with all other syntactic sugar. Segher
On Thu, May 10, 2018 at 05:20:52PM -0500, Segher Boessenkool wrote: > On Thu, May 10, 2018 at 05:49:12PM -0400, Michael Meissner wrote: > > > > -/* Return true if we have D-form addressing in altivec registers. */ > > > > +/* Return true if we have D-form addressing (register+offset) in either a > > > > + specific reload register class or whether some reload register class > > > > + supports d-form addressing. */ > > > > static inline bool > > > > -mode_supports_vmx_dform (machine_mode mode) > > > > +mode_supports_d_form (machine_mode mode, > > > > + enum rs6000_reload_reg_type rt = RELOAD_REG_ANY) > > > > { > > > > - return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0); > > > > + return ((reg_addr[mode].addr_mask[rt] & RELOAD_REG_OFFSET) != 0); > > > > } > > > > > > Will this overload help anything? It does not look that way, all current > > > callers use a different argument (and all the same). > > > > All current callers just use the ANY option (except for these calls). However > > in the future, I'm planning on calling these functions with the specific reload > > register class (hence the change). > > No, they use RELOAD_REG_VMX. Unless there is something extra tricky > about your patch? Yes, point is to make it more general. Right now, mode_supports_vmx_dform is only called to see if we have the ISA 3.0 d/ds/dq-form instructions. It is called in secondary reload to see if a register supports d*-form insns. It is also called in preferred reload class to say whether we would prefer just a FPR register for a d*-form insn or we can also tolerate a VMX register. However, as I contemplate reworking the memory support, I want to clean up a lot of the code that knows certain modes can do certain address forms. I'm getting to hate code like: if (mode == SFmode || mode == DFmode || ...) So I was just trying to clean things up, and migratate to using the reg_addr[mode].addr_mask bits. Having the inline functions can also make the line length smaller. -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 259864) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -649,7 +649,7 @@ mode_supports_vmx_dform (machine_mode mo is more limited than normal d-form addressing in that the offset must be aligned on a 16-byte boundary. */ static inline bool -mode_supports_vsx_dform_quad (machine_mode mode) +mode_supports_dq_form (machine_mode mode) { return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_QUAD_OFFSET) != 0); @@ -7922,7 +7922,7 @@ quad_address_p (rtx addr, machine_mode m if (legitimate_indirect_address_p (addr, strict)) return true; - if (VECTOR_MODE_P (mode) && !mode_supports_vsx_dform_quad (mode)) + if (VECTOR_MODE_P (mode) && !mode_supports_dq_form (mode)) return false; if (GET_CODE (addr) != PLUS) @@ -8104,7 +8104,7 @@ reg_offset_addressing_ok_p (machine_mode IEEE 128-bit floating point that is passed in a single vector register. */ if (VECTOR_MEM_ALTIVEC_OR_VSX_P (mode)) - return mode_supports_vsx_dform_quad (mode); + return mode_supports_dq_form (mode); break; case E_SDmode: @@ -8164,7 +8164,7 @@ offsettable_ok_by_alignment (rtx op, HOS /* ISA 3.0 vector d-form addressing is restricted, don't allow SYMBOL_REF. */ - if (mode_supports_vsx_dform_quad (mode)) + if (mode_supports_dq_form (mode)) return false; dsize = GET_MODE_SIZE (mode); @@ -8335,7 +8335,7 @@ rs6000_legitimate_offset_address_p (mach return false; if (!INT_REG_OK_FOR_BASE_P (XEXP (x, 0), strict)) return false; - if (mode_supports_vsx_dform_quad (mode)) + if (mode_supports_dq_form (mode)) return quad_address_p (x, mode, strict); if (!reg_offset_addressing_ok_p (mode)) return virtual_stack_registers_memory_p (x); @@ -8448,7 +8448,7 @@ legitimate_lo_sum_address_p (machine_mod if (!INT_REG_OK_FOR_BASE_P (XEXP (x, 0), strict)) return false; /* quad word addresses are restricted, and we can't use LO_SUM. */ - if (mode_supports_vsx_dform_quad (mode)) + if (mode_supports_dq_form (mode)) return false; x = XEXP (x, 1); @@ -8513,7 +8513,7 @@ rs6000_legitimize_address (rtx x, rtx ol unsigned int extra; if (!reg_offset_addressing_ok_p (mode) - || mode_supports_vsx_dform_quad (mode)) + || mode_supports_dq_form (mode)) { if (virtual_stack_registers_memory_p (x)) return x; @@ -9229,7 +9229,7 @@ rs6000_legitimize_reload_address (rtx x, int ind_levels ATTRIBUTE_UNUSED, int *win) { bool reg_offset_p = reg_offset_addressing_ok_p (mode); - bool quad_offset_p = mode_supports_vsx_dform_quad (mode); + bool quad_offset_p = mode_supports_dq_form (mode); /* Nasty hack for vsx_splat_v2df/v2di load from mem, which takes a DFmode/DImode MEM. Ditto for ISA 3.0 vsx_splat_v4sf/v4si. */ @@ -9515,7 +9515,7 @@ static bool rs6000_legitimate_address_p (machine_mode mode, rtx x, bool reg_ok_strict) { bool reg_offset_p = reg_offset_addressing_ok_p (mode); - bool quad_offset_p = mode_supports_vsx_dform_quad (mode); + bool quad_offset_p = mode_supports_dq_form (mode); /* If this is an unaligned stvx/ldvx type address, discard the outer AND. */ if (VECTOR_MEM_ALTIVEC_P (mode) @@ -19743,7 +19743,7 @@ rs6000_secondary_reload_inner (rtx reg, } } - else if (mode_supports_vsx_dform_quad (mode) && CONST_INT_P (op1)) + else if (mode_supports_dq_form (mode) && CONST_INT_P (op1)) { if (((addr_mask & RELOAD_REG_QUAD_OFFSET) == 0) || !quad_address_p (addr, mode, false)) @@ -19784,7 +19784,7 @@ rs6000_secondary_reload_inner (rtx reg, } /* Quad offsets are restricted and can't handle normal addresses. */ - else if (mode_supports_vsx_dform_quad (mode)) + else if (mode_supports_dq_form (mode)) { emit_insn (gen_rtx_SET (scratch, addr)); new_addr = scratch; @@ -19979,7 +19979,7 @@ rs6000_preferred_reload_class (rtx x, en /* D-form addressing can easily reload the value. */ if (mode_supports_vmx_dform (mode) - || mode_supports_vsx_dform_quad (mode)) + || mode_supports_dq_form (mode)) return rclass; /* If this is a scalar floating point value and we don't have D-form @@ -20382,7 +20382,7 @@ rs6000_output_move_128bit (rtx operands[ else if (TARGET_VSX && dest_vsx_p) { - if (mode_supports_vsx_dform_quad (mode) + if (mode_supports_dq_form (mode) && quad_address_p (XEXP (src, 0), mode, true)) return "lxv %x0,%1"; @@ -20420,7 +20420,7 @@ rs6000_output_move_128bit (rtx operands[ else if (TARGET_VSX && src_vsx_p) { - if (mode_supports_vsx_dform_quad (mode) + if (mode_supports_dq_form (mode) && quad_address_p (XEXP (dest, 0), mode, true)) return "stxv %x1,%0";