On Tue, Oct 18, 2016 at 01:18:42PM +0200, Bernd Schmidt wrote:
> On 10/17/2016 09:46 PM, [email protected] wrote:
> > {
> > - rtx r0, r16, eqv, tga, tp, insn, dest, seq;
> > + rtx r0, r16, eqv, tga, tp, dest, seq;
> > + rtx_insn *insn;
> >
> > switch (tls_symbolic_operand_type (x))
> > {
> > @@ -1025,66 +1026,70 @@ alpha_legitimize_address_1 (rtx x, rtx scratch,
> > machine_mode mode)
> > break;
> >
> > case TLS_MODEL_GLOBAL_DYNAMIC:
> > - start_sequence ();
> > + {
> > + start_sequence ();
> >
> > - r0 = gen_rtx_REG (Pmode, 0);
> > - r16 = gen_rtx_REG (Pmode, 16);
> > - tga = get_tls_get_addr ();
> > - dest = gen_reg_rtx (Pmode);
> > - seq = GEN_INT (alpha_next_sequence_number++);
> > + r0 = gen_rtx_REG (Pmode, 0);
> > + r16 = gen_rtx_REG (Pmode, 16);
> > + tga = get_tls_get_addr ();
> > + dest = gen_reg_rtx (Pmode);
> > + seq = GEN_INT (alpha_next_sequence_number++);
> >
> > - emit_insn (gen_movdi_er_tlsgd (r16, pic_offset_table_rtx, x, seq));
> > - insn = gen_call_value_osf_tlsgd (r0, tga, seq);
> > - insn = emit_call_insn (insn);
> > - RTL_CONST_CALL_P (insn) = 1;
> > - use_reg (&CALL_INSN_FUNCTION_USAGE (insn), r16);
> > + emit_insn (gen_movdi_er_tlsgd (r16, pic_offset_table_rtx, x, seq));
> > + rtx val = gen_call_value_osf_tlsgd (r0, tga, seq);
>
> Since this doesn't consistently declare variables at the point of
> initialization, might as well put val into the list of variables at the top,
> and avoid reindentation that way. There are several such reindented blocks,
> and the patch would be a lot easier to review without this.
I do really prefer reading code where variables are declared at first
use, but I'll agree with the tools we are using this can be hard to
review, sorry about that.
> Alternatively, split it up a bit more into obvious/nonobvious parts.
yeah, I'll try to get that done soon. fwiw a -b diff is below if you
find that better.
> > diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
> > index 21bba0c..8e8fff4 100644
> > --- a/gcc/config/arc/arc.c
> > +++ b/gcc/config/arc/arc.c
> > @@ -4829,7 +4829,6 @@ static rtx
> > arc_emit_call_tls_get_addr (rtx sym, int reloc, rtx eqv)
> > {
> > rtx r0 = gen_rtx_REG (Pmode, R0_REG);
> > - rtx insns;
> > rtx call_fusage = NULL_RTX;
> >
> > start_sequence ();
> > @@ -4846,7 +4845,7 @@ arc_emit_call_tls_get_addr (rtx sym, int reloc, rtx
> > eqv)
> > RTL_PURE_CALL_P (call_insn) = 1;
> > add_function_usage_to (call_insn, call_fusage);
> >
> > - insns = get_insns ();
> > + rtx_insn *insns = get_insns ();
> > end_sequence ();
>
> For example, stuff like this looks obvious enough that it can go in.
yeah, I think Jeff preapproved stuff like that a while back, but I just
lumped it in though really that wasn't very nice to review, and there
isn't really a reason for a human to review what a compiler can check
for us.
Thanks!
Trev
>
>
> Bernd
