On Mon, Oct 17, 2016 at 12:38:36PM +0000, Wilco Dijkstra wrote:
>
> ping
>
>
> From: Wilco Dijkstra
> Sent: 10 August 2016 17:20
> To: Richard Earnshaw; GCC Patches
> Cc: nd
> Subject: Re: [PATCH][AArch64] Improve stack adjustment
>
> Richard Earnshaw wrote:
> > I see you've added a default argument for your new parameter. I think
> > doing that is fine, but I have two comments about how we might use that
> > in this case.
> >
> > Firstly, if this parameter is suitable for having a default value, then
> > I think the preceding one should also be treated in the same way.
> >
> > Secondly, I think (due to these parameters being BOOL with no useful
> > context to make it clear which is which) that having wrapper functions
> > (inlined, of course) that describe the intended behaviour more clearly
> > would be useful. So, defining
> >
> > static inline void
> > aarch64_add_frame_constant (mode, regnum, scratchreg, delta)
> > {
> > aarch64_add_frame_constant (mode, regnum, scratchreg, delta, true);
> > }
> >
> > would make it clearer at the call point than having a lot of true and
> > false parameters scattered round the code.
> >
> > Alternatively we might remove all the default parameters and require
> > wrappers like the above to make it more explicit which API is intended -
> > this might make more sense if not all combinations make sense.
>
> OK here is the new version using simple wrapper functions and no
> default parameters:
I've got a simple suggestion for one of your comments, and I'd like
clarification on something I'm not understanding...
> gcc/
> * config/aarch64/aarch64.c (aarch64_add_constant_internal):
> Add extra argument to allow emitting the move immediate.
> Use add/sub with positive immediate.
> (aarch64_add_constant): Add inline function.
> (aarch64_add_sp): Likewise.
> (aarch64_sub_sp): Likewise.
> (aarch64_expand_prologue): Call aarch64_sub_sp.
> (aarch64_expand_epilogue): Call aarch64_add_sp.
> Decide when to leave out move.
> (aarch64_output_mi_thunk): Call aarch64_add_constant.
> @@ -1967,11 +1973,11 @@ aarch64_add_constant (machine_mode mode, int regnum,
> int scratchreg,
> return;
> }
>
> - /* We need two add/sub instructions, each one performing part of the
> - calculation. Don't do this if the addend can be loaded into register
> with
> - a single instruction, in that case we prefer a move to a scratch
> register
> - following by an addition. */
> - if (mdelta < 0x1000000 && !aarch64_move_imm (delta, mode))
> + /* We need two add/sub instructions, each one perform part of the
> + addition/subtraction, but don't this if the addend can be loaded into
> + register by single instruction, in that case we prefer a move to scratch
> + register following by addition. */
This sentence is missing some words.
> + if (mdelta < 0x1000000 && !aarch64_move_imm (mdelta, mode))
Could you explain this change? The comment makes it seem like delta would
still be correct. Probably the comment needs to say "followed by
addition/subtraction" rather than what is currently written?
> {
> HOST_WIDE_INT low_off = mdelta & 0xfff;
>
> @@ -1985,8 +1991,10 @@ aarch64_add_constant (machine_mode mode, int regnum,
> int scratchreg,
>
> /* Otherwise use generic function to handle all other situations. */
> rtx scratch_rtx = gen_rtx_REG (mode, scratchreg);
> - aarch64_internal_mov_immediate (scratch_rtx, GEN_INT (delta), true, mode);
> - insn = emit_insn (gen_add2_insn (this_rtx, scratch_rtx));
> + if (emit_move_imm)
> + aarch64_internal_mov_immediate (scratch_rtx, GEN_INT (mdelta), true,
> mode);
> + insn = emit_insn (delta < 0 ? gen_sub2_insn (this_rtx, scratch_rtx)
> + : gen_add2_insn (this_rtx, scratch_rtx));
What is contained in scratch_rtx here if we didn't set it up with
aarch64_internal_move_immediate? Are you not adding junk values in the
!emit_move_imm case?
Thanks,
James
> if (frame_related_p)
> {
> RTX_FRAME_RELATED_P (insn) = frame_related_p;
