[PATCH] PowerPC Prefixed Memory, Patch #3, Update predicates

[Bill Schmidt]

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
>