On Tue, Oct 06, 2015 at 09:39:20PM +0900, Oleg Endo wrote:
> 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.
Thanks!
> > + 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 ...
This is generating trampolines for nested functions. This portion of
the patch applied without modification from the old patch, so I didn't
read into it in any more detail; it seems to be the following, which
makes sense:
0: .long 1f
.long gotval
1: mov.l 3f,r3
mov.l 2f,r2
mov.l @r2,r1
mov.l @(4,r2),r12
jmp @r1
nop
3: .long cxt
2: .long fnaddr
The corresponding non-FDPIC version is:
mov.l 3f,r3
mov.l 2f,r2
jmp @r2
nop
3: .long cxt
2: .long fnaddr
Should these go into the source as comments?
> 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)
I would think it does, but I've found in the RTL files sometimes extra
escaping is silently accepted, and I'm not sure if omitting it would
visibly break. Can I rely on it producing a visible error right away
if removing it is wrong, or do I need to search the gccint
documentation to figure out what the right way is?
> > +@item --enable-fdpic
> > +On SH Linux systems, generate ELF FDPIC code.
>
> It should be "GNU/Linux" as far as I know.
I don't want to turn this into a political battle so we can go with
whatever is appropriate for upstream gcc. Note however that, at
present, the only targets this code is useful on are completely
non-GNU Linux (musl-based and not using any GNU userspace on the
target). uClibc may also work if someone digs up the old (untouched
since 2011) superh_fdpic branch.
The original patch said "On SH uClinux systems"; I just changed it to
"Linux" because there's no longer anything uClinux-specific about it.
> > 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.
By "better" you mean leaving the self-specs approach in-place but
explicitly initializing it to 0 with Init(0)? That sounds good to me.
> > - 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.
It can't generate the same code either way because, with the patch as
submitted, there's an extra load inside the asm. I would prefer
switching to an approach that avoids that (mainly to avoid the ugly
near-duplication of the asm block, but also to save a couple
instructions) but short of feedback on acceptable ways to do the
punning in the C++ I'll just leave it in the asm for now.
> > - 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.
OK. Actually I think the patch is a lot closer to the old one than you
think, or at least that's how it feels to me. But I don't mind working
through the whole new patch to make a better ChangeLog entry and just
using the old one as "source for inspiration".
Rich
