On Wed, Jul 10, 2019 at 12:11:49PM -0500, Segher Boessenkool wrote:
> Hi Mike,
>
> On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> > @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
> > static bool
> > use_toc_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));
> > + if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> > + return false;
>
> Why the SYMBOL_REF test? The original didn't have any. But its comment
> says
> /* 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. */
> so perhaps you want an assert instead.
The only two callers had a test for SYMBOL_REF_P. By moving it into the
function, it made the call to the function one line instead of two. But I can
go back to the previous method.
> > +
> > + if (constant_pool_expr_p (sym)
> > + && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > + get_pool_mode (sym)))
> > + return true;
> > +
> > + return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> > + && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);
>
> Please don't put disparate things on one line, it was fine before.
I'm not sure what you mean, I was just trying to break up a long if statement.
> > +/* Return true iff the given SYMBOL_REF refers to a constant pool entry
> > that we
> > + have put in the pc-relative constant area, or for cmodel=medium, if the
> > + SYMBOL_REF can be addressed via pc-relative instructions. */
> > +
> > +static bool
> > +use_pc_relative_ref (rtx sym)
> > +{
> > + if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
> > + return false;
>
> Same here, assert please. Or nothing, it will ICE not much later anyway.
> But don't silently return if something violates the call contract.
Again, this was meant to simplify the call.
> > -static GTY(()) alias_set_type set = -1;
> > +/* Return the alias set for constants stored in either the TOC or via
> > + pc-relative addressing. */
> > +static GTY(()) alias_set_type data_alias_set = -1;
>
> Please just make a separate alias set for pcrel. The new name isn't as
> bad as just "set" :-), but "data"? That's not great either. Conflating
> toc and pcrel isn't a good thing.
>
> (Variables don't "return" anything btw).
>
> >
> > alias_set_type
> > -get_TOC_alias_set (void)
> > +get_data_alias_set (void)
>
> This name is much worse than the old one. Just make separate functions
> for TOC and for pcrel? Or is there any reason you want to share them?
Well in theory, an object file that contains some functions wtih pc-relative
and some with TOC (due to #pragma target or attribute), using the same data set
would flag that they are related, but I imagine in practice the two uses don't
mix. It was more of a hangover from trying to have one function to create both
addressing forms.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
