This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH], PowerPC, Patch #6, Create pc-relative addressing insns
On Wed, Jul 10, 2019 at 01:56:07PM -0400, Michael Meissner wrote:
> On Wed, Jul 10, 2019 at 12:11:49PM -0500, Segher Boessenkool wrote:
> > 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.
It makes more sense with the param as a symbol always (the function
comment says it is, too, and the "sym" name suggests it is). So yes,
please go back to that. Reducing number of lines of code isn't a goal;
reducing complexity is, reducing astonishment.
> > > +
> > > + 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.
"TARGET_CMODEL == CMODEL_MEDIUM" and "SYMBOL_REF_LOCAL_P (sym)" are quite
different things.
> > > -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.
I think it would make things quite a bit simpler if you split them up.
Could you please try that?
Segher