Re: [PATCH v2] SH FDPIC backend support

[Oleg Endo <oleg dot endo at t-online dot de>]

On Mon, 2015-10-05 at 23:15 -0400, Rich Felker wrote:
> Attached is the initial version of the patch against trunk. I've fixed
> the functional issues I'm aware of from the previous version: ICE in
> generating the plain-SH2 libgcc-based shifts, missing
> sh_legitimate_constant_p changes, and bad asm spec that broke
> non-FDPIC. Cosmetic/style changes have not been made yet.

OK, I've got a few other points.

> +  if (TARGET_FDPIC)
> +    {
> +      rtx a = force_reg (Pmode, plus_constant (Pmode, XEXP (tramp_mem, 0), 8));
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 0), a);
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 4), OUR_FDPIC_REG);
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 8),
> +		      gen_int_mode (TARGET_LITTLE_ENDIAN ? 0xd203d302 : 0xd302d203,
> +				    SImode));
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 12),
> +		      gen_int_mode (TARGET_LITTLE_ENDIAN ? 0x5c216122 : 0x61225c21,
> +				    SImode));
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 16),
> +		      gen_int_mode (TARGET_LITTLE_ENDIAN ? 0x0009412b : 0x412b0009,
> +				    SImode));
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 20), cxt);
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 24), fnaddr);
> +    }
> +  else
> +    {
> +      emit_move_insn (change_address (tramp_mem, SImode, NULL_RTX),
> +		      gen_int_mode (TARGET_LITTLE_ENDIAN ? 0xd301d202 : 0xd202d301,
> +				    SImode));
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 4),
> +		      gen_int_mode (TARGET_LITTLE_ENDIAN ? 0x0009422b : 0x422b0009,
> +				    SImode));
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 8), cxt);
> +      emit_move_insn (adjust_address (tramp_mem, SImode, 12), fnaddr);
> +    }

I think this hunk really needs a comment.  It copies machine code from
somewhere to somewhere via constant loads... but what exactly are the
instructions ...

In the multiple alternative insn patterns ...

> -  "jsr	@%1%#"
> +  "@
> +   jsr	@%1%#
> +   bsrf	%1\\n%O2:%#"

Please use the formatting as in the other parts of sh.md:
"@
	jsr	@%1%#
	bsrf	%1\n%O2:%#"

(use tabs and I don't think the embedded newline needs double backslash,
but please check)


> +@item --enable-fdpic
> +On SH Linux systems, generate ELF FDPIC code.

It should be "GNU/Linux" as far as I know.

> 
> A couple specific questions I have:
> 
> - Is the use of self specs (see DRIVER_SELF_SPECS in sh.h) an
>   acceptable way to set the default? I brought this up before but
>   don't think anyone answered. I find this method more clear and less
>   invasive (doesn't require #ifdef FDPIC_DEFAULT all over the place)
>   but if there's a policy reason this can't be done I can rework it.

This should be fine.  If there's a problem with that it can be changed
later.  Kaz, do you have any opinion?

Still, maybe it's better ...

> mfdpic
> Target Report Var(TARGET_FDPIC)
> Generate ELF FDPIC code

... to add an Init(0) to this new option.  Just in case.

> - For the udiv_qrnnd inline asm, the current patch duplicates the asm
>   with a minor change to dereference the function descriptor and get a
>   code address. This could be done outside the asm (via type punning
>   the function pointer) to slightly improve the resulting code and
>   avoid duplicating the asm (a macro would be used to load the code
>   address from the function pointer; this is identity macro on
>   non-FDPIC and would do the type punning on FDPIC) but if this
>   approach would be preferable I need some advice on the form of type
>   punning that would be acceptable in GCC.

I think this is fine as it is.  udiv_qrnnd is probably not used very
often, so most likely the compiler will generate the same code to do a
constant pool load before the asm block.  If more such functions are
introduced it might be worth doing it.

> - For the Changelog, should I just edit the one from the original
>   patch (https://gcc.gnu.org/ml/gcc-patches/2010-08/txt00148.txt)
>   submitted against 4.5 and add myself to the list of patch authors?

I think a new ChangeLog entry is better, since the patch most likely
will look quite different from the original.  You can use the original
text as a source for inspiration.  Giving credit to the original authors
would be nice I think.

Cheers,
Oleg