Re: [PATCH], V4.1, patch #2: Add prefixed insn attribute (revised)

[Segher Boessenkool <segher at kernel dot crashing dot org>]

Hi!

On Tue, Sep 24, 2019 at 02:10:10AM -0400, Michael Meissner wrote:
> +/* Return true if the address is a prefixed instruction that can be directly
> +   used in a memory instruction (i.e. using numeric offset or a PC-relative
> +   reference to a local symbol).

This could use a bit of a rewrite...  "Return whether the address is valid
for a prefixed memory instruction [...]"?

> +      /* Use the default pattern for loading up PC-relative addresses.  */
> +      if (TARGET_PCREL && mode == Pmode
> +	  && (SYMBOL_REF_P (operands[1]) || LABEL_REF_P (operands[1])
> +	      || GET_CODE (operands[1]) == CONST))

Maybe this can use some predicate function?  That will make the CONST
stand out more as being the special case here, too?

> +  unsigned int r = reg_or_subregno (reg);
> +
> +  /* If we have a pseudo, use the default instruction format.  */
> +  if (r >= FIRST_PSEUDO_REGISTER)
> +    return NON_PREFIXED_DEFAULT;

Please use

  if (!HARD_REGISTER_NUM_P (r))

> +	 (eq_attr "type" "load,fpload,vecload")
> +	 (if_then_else (and (eq_attr "indexed" "no")
> +			    (eq_attr "update" "no")
> +			    (match_test "prefixed_load_p (insn)"))
> +		       (const_string "yes")
> +		       (const_string "no"))

It looks like prefixed_load_p and prefixed_store_p should test for
"indexed" "no" and "update" "no" themselves?  The code here simplifies
a bit then.

(blank line before the default case please).

> +	(const_string "no")))


Okay for trunk with those things fixed.  Thanks!


Segher