This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH], PowerPC, Patch #6, revision 2, Create pc-relative addressing insns


On Thu, Jul 11, 2019 at 04:09:44PM -0400, Michael Meissner wrote:
> On Thu, Jul 11, 2019 at 02:52:03PM -0500, Segher Boessenkool wrote:
> > On Wed, Jul 10, 2019 at 05:17:48PM -0400, Michael Meissner wrote:
> > > +extern alias_set_type get_pc_relative_alias_set (void);
> > 
> > I'd just call it "pcrel", not "pc_relative", just like everywhere else?
> > 
> > > @@ -7785,7 +7787,7 @@ bool
> > >  toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,
> > >  		     const_rtx *tocrel_offset_ret)
> > >  {
> > > -  if (!TARGET_TOC)
> > > +  if (!TARGET_TOC || TARGET_PCREL)
> > 
> > Maybe (TARGET_TOC && !TARGET_PCREL) should get a name of its own?  Or
> > maybe TARGET_TOC should not be enabled when TARGET_PCREL is?  It doesn't
> > make much sense at all to have both enabled, does it?
> 
> Yeah, in theory TARGET_TOC should not be set if TARGET_PCREL is set, but when I
> last tried it, there were some failures.  I can make a macro for the two together.

TARGET_TOC is set in various header files (linux64.h etc.)  While
TARGET_PCREL is defined in rs6000.opt .  Maybe rename the original
TARGET_TOC definitions, and make a generic TARGET_TOC defined as
(TARGET_CAN_HAVE_TOC && !TARGET_PCREL) ?  Something like that?

> > > +      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])
> > > +	       && TARGET_CMODEL == CMODEL_MEDIUM
> > > +	       && SYMBOL_REF_LOCAL_P (operands[1]))
> > 
> >       else if (TARGET_PCREL
> > 	       && TARGET_CMODEL == CMODEL_MEDIUM
> > 	       && SYMBOL_REF_P (operands[1])
> > 	       && SYMBOL_REF_LOCAL_P (operands[1]))

> I had it in a function that did the checking, but after eliminating the
> constant pool check that TOC needs, it was just doing the medium code model and
> symbol_ref local checks, so I eliminated the function.

My point is that you should put like things together, and you should not
put unrelated conditions on one line.

You could put the SYMBOL_REF_P and SYMBOL_REF_LOCAL_P on one line, I
wouldn't mind (except that doesn't fit on one line then ;-) ).

> > >  alias_set_type
> > >  get_TOC_alias_set (void)
> > >  {
> > > -  if (set == -1)
> > > -    set = new_alias_set ();
> > > -  return set;
> > > +  if (TOC_alias_set == -1)
> > > +    TOC_alias_set = new_alias_set ();
> > > +  return TOC_alias_set;
> > > +}
> > 
> > It would be nice if you could initialise the alias sets some other way.
> > Not new code, but you duplicate it ;-)
> 
> For the pc-relative case, I'm not sure we need the alias set.  In the TOC case,
> we do a force_const_mem which returns a SYMBOL_REF pointing to the data in the
> constant pool.  Then we call create_TOC_reference which rewrites the memory,
> and then it calls gen_const_mem which notes that memory in unchanging.
> 
> But for pc-relative, we just use output of force_const_mem.  Now
> force_const_mem does not seem to set the alias set (but it will have the
> constant pool bit set).

I mean just this:
  if (TOC_alias_set == -1)
    TOC_alias_set = new_alias_set ();
in the getter function.  It could be initialised elsewhere, there are
enough initialisation functions, it will fit *somewhere*, and then the
getter function doesn't need to do the initialisation anymore.  Which
then suddenly makes it very inlinable, etc.


Segher


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]