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
