On Fri, Jun 28, 2019 at 08:20:35AM -0500, Segher Boessenkool wrote:
> Hi Mike,
>
> On Thu, Jun 27, 2019 at 08:12:35PM -0400, Michael Meissner wrote:
> > + return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op)
> > + && SYMBOL_REF_LOCAL_P (op));
>
> Please break the line before that first && as well?
Ok.
> > +(define_predicate "pcrel_external_address"
> > + (match_code "symbol_ref,const")
> > +{
> > + if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM)
> > + return false;
>
> if (!(TARGET_PCREL && TARGET_CMODEL == CMODEL_MEDIUM))
>
> Should there be a helper function for this? PCREL is only supported
> for medium model -- abstracting that makes both the current code easier
> to read, and if we ever want to support other models, that will be
> simpler to do as well.
Yeah, Bill's suggestion is probably what I should use.
> > + /* Discard any CONST's. */
> > + if (GET_CODE (op) == CONST)
> > + op = XEXP (op, 0);
>
> That comment says nothing the code already tells. It's a common thing
> to do anyway; just don't comment on it?
Ok.
> > +;; Return 1 if op is a memory operand to an external variable when we
> > +;; support pc-relative addressing and the PCREL_OPT relocation to
> > +;; optimize references to it.
> > +(define_predicate "pcrel_external_mem_operand"
> > + (match_code "mem")
> > +{
> > + return pcrel_external_address (XEXP (op, 0), Pmode);
> > +})
>
> Is that comment correct? pcrel_external_address does nothing with
> PCREL_OPT? Or _its_ comment doesn't say, at least.
Yes, I will re-word it. We will need this predicate even if PCREL_OPT is not
being used.
> Okay for trunk with those trivialities resolved. Thanks!
>
>
> Segher
--
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
