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
