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
