On 6/28/19 8:20 AM, 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?
>
>> +(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.
I'd suggest
if (!rs6000_pcrel_p ())
which will also make sure this is ELFv2.
Thanks,
Bill
>
>> + /* 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?
>
>> +;; 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.
>
> Okay for trunk with those trivialities resolved. Thanks!
>
>
> Segher
>
