On Fri, Oct 02, 2015 at 10:51:03PM +0900, Oleg Endo wrote:
> 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.
Thanks! This is very helpful. gcc style has changed a lot since the
old patch was submitted so I think it makes sense to update it to
match current practices rather than just making it work. I'll try to
focus on any functional problems first though so as to keep a working
patch against 5.2 as well and ease backporting to earlier versions (if
anyone wants to do that on their own; certainly I don't expect it to
happen in upstream gcc).
> > +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.
OK.
> 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.
There are a few call sites where the symbol returned is actually used.
Would you want me to just do something like:
struct function_symbol_result funcsym = function_symbol(...);
then use funcsym.symbol and funcsym.label?
Would you object to shorter member names .sym and .lab?
> > +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.
Uhg, not sure how I missed that. (Well, yes I am -- it's C++'s
fault;-) I'll try to figure out what's going on.
> > +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'.
OK.
> > +/* 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.
OK.
> > void
> > prepare_move_operands (rtx operands[], machine_mode mode)
> > {
> > + rtx tmp, base, offset;
> > +
>
> Please declare variables where they are used.
OK.
> > + 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 ());
OK.
> > +(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 ..
Because SH5 is deprecated?
> > + 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.
OK, but I'm not really familiar with this part of the code; I just
adapted the patch by pattern. There are a lot of places with
(match_operand N "" ""); should the empty strings be dropped for all
of them?
> > if (TARGET_FDPIC)
> > + picreg = OUR_FDPIC_REG;
> > + else
> > + picreg = gen_rtx_REG (Pmode, PIC_REG);
> > +
>
> rtx picreg = TARGET_FDPIC ? ...
> : ... ;
OK.
> 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.
I'm fairly indifferent to this. Neither is significantly shorter or
more readable.
> > +// 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.
I'm a bit confused by the fact that basically ALL of the traffic on
the linux-sh list is "shmedia" stuff. Is that unrelated to the actual
SH5/SHMEDIA and just a brand name that got co-opted for an ARM-based
SoC? If so, is there anything that can be done to get it off the
linux-sh list so that it doesn't bury mail about the actual SH
ISA/platform?
> > +#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.
OK, I can add a comment; this is appropriate anyway since the way it's
making the FDPIC call is unconventional.
Rich
