[PATCH] SH FDPIC backend support

[Oleg Endo]

On Thu, 2015-10-01 at 21:30 -0400, Rich Felker wrote:

> If you have any other general comments on the patch in the mean time
> I'd be happy to hear them.

Below are some comments.  Might be a bit unstructured, I was hopping
through the patch file.  Sorry about that.

> +function_symbol (rtx target, const char *name, enum sh_function_kind kind, rtx *lab)
                                                 ^^^^^

Please do not add unnecessary 'enum', 'struct', 'typedef' etc.  In this
case it was already here, but since this is is touching the line, please
remove it.

I'd rather make the function 'function_symbol' returning a
std::pair<rtx,rtx> or something like

struct function_symbol_result
{
  function_symbol_result (void) : symbol (NULL), label (NULL) { }
  function_symbol_result (rtx s, rtx l) : symbol (s), label (l) { }

  rtx symbol;
  rtx label;
};

instead of doing return values by pointer-args.  On the caller sites,
you can then do something like

rtx lab = function_symbol (func_addr_rtx, "...", SFUNC_STATIC).label;

This will make the the patch also a few hunks shorter.

> +extern bool sh_legitimate_constant_p (rtx);

There is already a target hook/callback function:

static bool
sh_legitimate_constant_p (machine_mode mode, rtx x)

You newly added function is an overload it and I'm not sure who invokes it.


> +extern rtx sh_our_fdpic_reg (void);

Please rename this to 'sh_get_fdpic_reg_initial_val'.  There's a similar
function 'sh_get_pr_initial_val' which also uses
'get_hard_reg_initial_val'.

> +/* An rtx holding the initial value of the FDPIC register (the FDPIC
> +   pointer passed in from the caller).  */
> +#define OUR_FDPIC_REG		sh_our_fdpic_reg ()
> +

Please remove this macro and add 'sh_get_fdpic_reg_initial_val' to
sh-protos.h and use that function instead.

>  void
>  prepare_move_operands (rtx operands[], machine_mode mode)
>  {
> +  rtx tmp, base, offset;
> +

Please declare variables where they are used.


> +  if (TARGET_FDPIC)
> +    {
> +      rtx pic_reg = gen_rtx_REG (Pmode, PIC_REG);
> +      emit_move_insn (pic_reg, OUR_FDPIC_REG);
> +    }
> +

Make this a one-liner

  emit_move_insn (gen_rtx_REG (Pmode, PIC_REG),
		  sh_get_fdpic_reg_initial_val ());



> +(define_insn "sibcalli_fdpic"
> +  [(call (mem:SI (match_operand:SI 0 "register_operand" "k"))
> +	 (match_operand 1 "" ""))
> +   (use (reg:SI FPSCR_MODES_REG))
> +   (use (reg:SI PIC_REG))
> +   (return)]
> +  "TARGET_SH1 && TARGET_FDPIC"
>            ^^^

This is maybe slightly impossible, because of ..

> +  if (TARGET_FDPIC
> +      && (TARGET_SHMEDIA || TARGET_SHCOMPACT || !TARGET_SH2))
> +    sorry ("non-SH2 FDPIC");
> +


> +  [(match_operand 0 "" "") (match_operand 1 "" "")]
> 

Please don't add empty predicate/constraint strings if not necessary.  In this case
	[(match_operand 0) (match_operand 1)]

will suffice.


>   if (TARGET_FDPIC)
> +    picreg = OUR_FDPIC_REG;
> +  else
> +    picreg = gen_rtx_REG (Pmode, PIC_REG);
> +

rtx picreg = TARGET_FDPIC ? ...
			  : ... ;

Maybe it could be useful to replace all "gen_rtx_REG (Pmode, PIC_REG)"
in the patch with something like 'get_t_reg_rtx'.  Depends on how many
times this gen_rtx_REG is invoked.


> +// FIXME: what happens if someone tries fdpic on SH5?
> 

Nothing.  See also
https://gcc.gnu.org/ml/gcc/2015-08/msg00101.html

Please omit all SH5/SHMEDIA checks and related code.


> +#ifdef __FDPIC__
> +#define udiv_qrnnd(q, r, n1, n0, d) \
> +  do {									\
> +    extern UWtype __udiv_qrnnd_16 (UWtype, UWtype)			\

It's really difficult to spot the subtle difference of the FDPIC version
and the non-FDPIC version.  At least there should be a comment.

Cheers,
Oleg