| Message ID | 20190628185033.GA32553@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | PowerPC Prefixed Memory, Patch #4, Add pc-relative reference support | expand |
Hi Mike, On Fri, Jun 28, 2019 at 02:50:33PM -0400, Michael Meissner wrote: > --- gcc/config/rs6000/rs6000-logue.c (revision 272714) > +++ gcc/config/rs6000/rs6000-logue.c (working copy) > @@ -1406,23 +1406,13 @@ uses_TOC (void) > } > #endif > > +/* Create a TOC style reference for a symbol. */ > rtx > create_TOC_reference (rtx symbol, rtx largetoc_reg) Does this really belong in this file? It doesn't do anythin *logue, and it isn't called from anywhere in here. > +/* Create either a TOC reference to a locally defined item or a pc-relative > + reference, depending on the ABI. */ > +rtx > +create_data_reference (rtx symbol, rtx largetoc_reg) Same here. What is largetoc_reg? The function comment should say. It also is only relevant for create_TOC_reference (where such a comment is also missing), so could you factor this better please? Probably a create_data_reference with only one argument? Which calls create_TOC_reference with a NULL second arg. It looks like your proposed create_data_reference will not do the right thing if called with a non-null second arg if pcrel. Perhaps that cannot happen, but make that clear then? Just an assert will do, bigger cleanups are better of course. Segher
On Wed, Jul 03, 2019 at 05:09:57PM -0500, Segher Boessenkool wrote: > Hi Mike, > > On Fri, Jun 28, 2019 at 02:50:33PM -0400, Michael Meissner wrote: > > --- gcc/config/rs6000/rs6000-logue.c (revision 272714) > > +++ gcc/config/rs6000/rs6000-logue.c (working copy) > > @@ -1406,23 +1406,13 @@ uses_TOC (void) > > } > > #endif > > > > +/* Create a TOC style reference for a symbol. */ > > rtx > > create_TOC_reference (rtx symbol, rtx largetoc_reg) > > Does this really belong in this file? It doesn't do anythin *logue, and > it isn't called from anywhere in here. Note, when rs6000-logue.c was created, it was put there. I was just trying to make the smallest number of changes to the infrastructure. I can move it back to rs6000.c if preferred. > > +/* Create either a TOC reference to a locally defined item or a pc-relative > > + reference, depending on the ABI. */ > > +rtx > > +create_data_reference (rtx symbol, rtx largetoc_reg) > > Same here. > > What is largetoc_reg? The function comment should say. It also is only > relevant for create_TOC_reference (where such a comment is also missing), > so could you factor this better please? Again, this was existing code, and I didn't really change the existing code. > Probably a create_data_reference with only one argument? Which calls > create_TOC_reference with a NULL second arg. It looks like your > proposed create_data_reference will not do the right thing if called > with a non-null second arg if pcrel. Perhaps that cannot happen, but > make that clear then? Just an assert will do, bigger cleanups are > better of course.
On Mon, Jul 08, 2019 at 02:42:13PM -0400, Michael Meissner wrote: > On Wed, Jul 03, 2019 at 05:09:57PM -0500, Segher Boessenkool wrote: > > Hi Mike, > > > > On Fri, Jun 28, 2019 at 02:50:33PM -0400, Michael Meissner wrote: > > > --- gcc/config/rs6000/rs6000-logue.c (revision 272714) > > > +++ gcc/config/rs6000/rs6000-logue.c (working copy) > > > @@ -1406,23 +1406,13 @@ uses_TOC (void) > > > } > > > #endif > > > > > > +/* Create a TOC style reference for a symbol. */ > > > rtx > > > create_TOC_reference (rtx symbol, rtx largetoc_reg) > > > > Does this really belong in this file? It doesn't do anythin *logue, and > > it isn't called from anywhere in here. > > Note, when rs6000-logue.c was created, it was put there. I know. That doesn't make it right though! :-) > I was just trying to > make the smallest number of changes to the infrastructure. I can move it back > to rs6000.c if preferred. Please do; as a separate patch. Thanks in advance. A patch purely moving it back is pre-approved. > > > +/* Create either a TOC reference to a locally defined item or a pc-relative > > > + reference, depending on the ABI. */ > > > +rtx > > > +create_data_reference (rtx symbol, rtx largetoc_reg) > > > > Same here. > > > > What is largetoc_reg? The function comment should say. It also is only > > relevant for create_TOC_reference (where such a comment is also missing), > > so could you factor this better please? > > Again, this was existing code, and I didn't really change the existing code. No, create_data_reference is a new function. That's the point. For create_TOC_reference it might make some sense (but it should be documented what this does); for create_data_reference it is a non-sensical parameter: either all callers use NULL, or you more likely than not have bugs. > > Probably a create_data_reference with only one argument? Which calls > > create_TOC_reference with a NULL second arg. It looks like your > > proposed create_data_reference will not do the right thing if called > > with a non-null second arg if pcrel. Perhaps that cannot happen, but > > make that clear then? Just an assert will do, bigger cleanups are > > better of course. Segher
Index: gcc/config/rs6000/rs6000-logue.c =================================================================== --- gcc/config/rs6000/rs6000-logue.c (revision 272714) +++ gcc/config/rs6000/rs6000-logue.c (working copy) @@ -1406,23 +1406,13 @@ uses_TOC (void) } #endif +/* Create a TOC style reference for a symbol. */ rtx create_TOC_reference (rtx symbol, rtx largetoc_reg) { rtx tocrel, tocreg, hi; - if (TARGET_DEBUG_ADDR) - { - if (SYMBOL_REF_P (symbol)) - fprintf (stderr, "\ncreate_TOC_reference, (symbol_ref %s)\n", - XSTR (symbol, 0)); - else - { - fprintf (stderr, "\ncreate_TOC_reference, code %s:\n", - GET_RTX_NAME (GET_CODE (symbol))); - debug_rtx (symbol); - } - } + gcc_assert (TARGET_TOC && !TARGET_PCREL); if (!can_create_pseudo_p ()) df_set_regs_ever_live (TOC_REGISTER, true); @@ -1441,6 +1431,39 @@ create_TOC_reference (rtx symbol, rtx la return gen_rtx_LO_SUM (Pmode, hi, tocrel); } +/* Create either a TOC reference to a locally defined item or a pc-relative + reference, depending on the ABI. */ +rtx +create_data_reference (rtx symbol, rtx largetoc_reg) +{ + if (TARGET_DEBUG_ADDR) + { + const char *toc_or_pcrel = (TARGET_PCREL) ? "pc-relative" : "TOC"; + + if (SYMBOL_REF_P (symbol)) + fprintf (stderr, "\ncreate_data_reference, abi %s, (symbol_ref %s)\n", + toc_or_pcrel, XSTR (symbol, 0)); + else + { + fprintf (stderr, "\ncreate_data_reference, abi %s, code %s:\n", + toc_or_pcrel, GET_RTX_NAME (GET_CODE (symbol))); + debug_rtx (symbol); + } + } + + /* We don't have to do anything special for pc-relative references. The + SYMBOL_REF bits sayss whether the label is local where we can use + pc-relative addressing, or if we have to load the address from the GOT + section to use it on external addresses. */ + if (TARGET_PCREL) + { + gcc_assert (TARGET_CMODEL == CMODEL_MEDIUM); + return symbol; + } + + return create_TOC_reference (symbol, largetoc_reg); +} + /* Issue assembly directives that create a reference to the given DWARF FRAME_TABLE_LABEL from the current function section. */ void Index: gcc/config/rs6000/rs6000-protos.h =================================================================== --- gcc/config/rs6000/rs6000-protos.h (revision 272714) +++ gcc/config/rs6000/rs6000-protos.h (working copy) @@ -131,6 +131,7 @@ extern void rs6000_emit_swdiv (rtx, rtx, extern void rs6000_emit_swsqrt (rtx, rtx, bool); extern void output_toc (FILE *, rtx, int, machine_mode); extern void rs6000_fatal_bad_address (rtx); +extern rtx create_data_reference (rtx, rtx); extern rtx create_TOC_reference (rtx, rtx); extern void rs6000_split_multireg_move (rtx, rtx); extern void rs6000_emit_le_vsx_permute (rtx, rtx, machine_mode); Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 272766) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -8084,7 +8084,7 @@ rs6000_legitimize_address (rtx x, rtx ol && SYMBOL_REF_P (x) && constant_pool_expr_p (x) && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (x), Pmode)) - return create_TOC_reference (x, NULL_RTX); + return create_data_reference (x, NULL_RTX); else return x; } @@ -8696,19 +8696,21 @@ rs6000_cannot_force_const_mem (machine_m return TARGET_ELF && tls_referenced_p (x); } -/* Return true iff the given SYMBOL_REF refers to a constant pool entry - that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF - can be addressed relative to the toc pointer. */ +/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we + have put in the TOC or is referenced via a pc-relative reference. In + addition, for cmodel=medium, if the SYMBOL_REF can be addressed relative to + the either toc pointer or via a pc-relative reference. */ static bool -use_toc_relative_ref (rtx sym, machine_mode mode) +use_toc_or_pc_relative_ref (rtx sym, machine_mode mode) { return ((constant_pool_expr_p (sym) && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym), get_pool_mode (sym))) || (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym) - && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT)); + && (TARGET_PCREL + || GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT))); } /* TARGET_LEGITIMATE_ADDRESS_P recognizes an RTL expression @@ -9810,8 +9812,8 @@ rs6000_emit_move (rtx dest, rtx source, reference to it. */ if (TARGET_TOC && SYMBOL_REF_P (operands[1]) - && use_toc_relative_ref (operands[1], mode)) - operands[1] = create_TOC_reference (operands[1], operands[0]); + && use_toc_or_pc_relative_ref (operands[1], mode)) + operands[1] = create_data_reference (operands[1], operands[0]); else if (mode == Pmode && CONSTANT_P (operands[1]) && GET_CODE (operands[1]) != HIGH @@ -9864,11 +9866,11 @@ rs6000_emit_move (rtx dest, rtx source, if (TARGET_TOC && SYMBOL_REF_P (XEXP (operands[1], 0)) - && use_toc_relative_ref (XEXP (operands[1], 0), mode)) + && use_toc_or_pc_relative_ref (XEXP (operands[1], 0), mode)) { - rtx tocref = create_TOC_reference (XEXP (operands[1], 0), - operands[0]); - operands[1] = gen_const_mem (mode, tocref); + rtx dataref = create_data_reference (XEXP (operands[1], 0), + operands[0]); + operands[1] = gen_const_mem (mode, dataref); set_mem_alias_set (operands[1], get_TOC_alias_set ()); } } Index: gcc/config/rs6000/rs6000.md =================================================================== --- gcc/config/rs6000/rs6000.md (revision 272757) +++ gcc/config/rs6000/rs6000.md (working copy) @@ -11666,9 +11666,9 @@ (define_insn_and_split "*cmp<mode>_inter if (TARGET_TOC) { rtx tocref; - tocref = create_TOC_reference (XEXP (operands[14], 0), operands[11]); + tocref = create_data_reference (XEXP (operands[14], 0), operands[11]); operands[14] = gen_const_mem (DFmode, tocref); - tocref = create_TOC_reference (XEXP (operands[15], 0), operands[11]); + tocref = create_data_reference (XEXP (operands[15], 0), operands[11]); operands[15] = gen_const_mem (DFmode, tocref); set_mem_alias_set (operands[14], get_TOC_alias_set ()); set_mem_alias_set (operands[15], get_TOC_alias_set ());