Re: [AArch64][3/3] Migrate aarch64_expand_prologue/epilogue to aarch64_add_constant

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 20/07/16 14:03, Jiong Wang wrote:
> Those stack adjustment sequences inside aarch64_expand_prologue/epilogue
> are doing exactly what's aarch64_add_constant offered, except they also
> need to be aware of dwarf generation.
> 
> This patch teach existed aarch64_add_constant about dwarf generation and
> currently SP register is supported.  Whenever SP is updated, there
> should be CFA update, we then mark these instructions as frame related,
> and if the update is too complex for gcc to guess the adjustment, we
> attach explicit annotation.
> 
> Both dwarf frame info size and pro/epilogue scheduling are improved after
> this patch as aarch64_add_constant has better utilization of scratch
> register.
> 
> OK for trunk?
> 
> gcc/
> 2016-07-20  Jiong Wang  <jiong.wang@arm.com>
> 
>             * config/aarch64/aarch64.c (aarch64_add_constant): Mark
>             instruction as frame related when it is.  Generate CFA
>             annotation when it's necessary.
>             (aarch64_expand_prologue): Use aarch64_add_constant.
>             (aarch64_expand_epilogue): Likewise.
> 

Are you sure using aarch64_add_constant is unconditionally safe?  Stack
adjustments need to be done very carefully to ensure that we never
transiently deallocate part of the stack.

R.


> 
> build-const-3.patch
> 
> 
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 41844a101247c939ecb31f8a8c17cf79759255aa..b38f3f1e8f85a5f3191d0c96080327dac7b2eaed 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -1874,6 +1874,8 @@ aarch64_add_constant (machine_mode mode, int regnum, int scratchreg,
>  {
>    HOST_WIDE_INT mdelta = abs_hwi (delta);
>    rtx this_rtx = gen_rtx_REG (mode, regnum);
> +  bool frame_related_p = (regnum == SP_REGNUM);
> +  rtx_insn *insn;
>  
>    /* Do nothing if mdelta is zero.  */
>    if (!mdelta)
> @@ -1882,7 +1884,8 @@ aarch64_add_constant (machine_mode mode, int regnum, int scratchreg,
>    /* We only need single instruction if the offset fit into add/sub.  */
>    if (aarch64_uimm12_shift (mdelta))
>      {
> -      emit_insn (gen_add2_insn (this_rtx, GEN_INT (delta)));
> +      insn = emit_insn (gen_add2_insn (this_rtx, GEN_INT (delta)));
> +      RTX_FRAME_RELATED_P (insn) = frame_related_p;
>        return;
>      }
>  
> @@ -1895,15 +1898,23 @@ aarch64_add_constant (machine_mode mode, int regnum, int scratchreg,
>        HOST_WIDE_INT low_off = mdelta & 0xfff;
>  
>        low_off = delta < 0 ? -low_off : low_off;
> -      emit_insn (gen_add2_insn (this_rtx, GEN_INT (low_off)));
> -      emit_insn (gen_add2_insn (this_rtx, GEN_INT (delta - low_off)));
> +      insn = emit_insn (gen_add2_insn (this_rtx, GEN_INT (low_off)));
> +      RTX_FRAME_RELATED_P (insn) = frame_related_p;
> +      insn = emit_insn (gen_add2_insn (this_rtx, GEN_INT (delta - low_off)));
> +      RTX_FRAME_RELATED_P (insn) = frame_related_p;
>        return;
>      }
>  
>    /* 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);
> -  emit_insn (gen_add2_insn (this_rtx, scratch_rtx));
> +  insn = emit_insn (gen_add2_insn (this_rtx, scratch_rtx));
> +  if (frame_related_p)
> +    {
> +      RTX_FRAME_RELATED_P (insn) = frame_related_p;
> +      rtx adj = plus_constant (mode, this_rtx, delta);
> +      add_reg_note (insn , REG_CFA_ADJUST_CFA, gen_rtx_SET (this_rtx, adj));
> +    }
>  }
>  
>  static bool
> @@ -3038,36 +3049,7 @@ aarch64_expand_prologue (void)
>        frame_size -= (offset + crtl->outgoing_args_size);
>        fp_offset = 0;
>  
> -      if (frame_size >= 0x1000000)
> -	{
> -	  rtx op0 = gen_rtx_REG (Pmode, IP0_REGNUM);
> -	  emit_move_insn (op0, GEN_INT (-frame_size));
> -	  insn = emit_insn (gen_add2_insn (stack_pointer_rtx, op0));
> -
> -	  add_reg_note (insn, REG_CFA_ADJUST_CFA,
> -			gen_rtx_SET (stack_pointer_rtx,
> -				     plus_constant (Pmode, stack_pointer_rtx,
> -						    -frame_size)));
> -	  RTX_FRAME_RELATED_P (insn) = 1;
> -	}
> -      else if (frame_size > 0)
> -	{
> -	  int hi_ofs = frame_size & 0xfff000;
> -	  int lo_ofs = frame_size & 0x000fff;
> -
> -	  if (hi_ofs)
> -	    {
> -	      insn = emit_insn (gen_add2_insn
> -				(stack_pointer_rtx, GEN_INT (-hi_ofs)));
> -	      RTX_FRAME_RELATED_P (insn) = 1;
> -	    }
> -	  if (lo_ofs)
> -	    {
> -	      insn = emit_insn (gen_add2_insn
> -				(stack_pointer_rtx, GEN_INT (-lo_ofs)));
> -	      RTX_FRAME_RELATED_P (insn) = 1;
> -	    }
> -	}
> +      aarch64_add_constant (Pmode, SP_REGNUM, IP0_REGNUM, -frame_size);
>      }
>    else
>      frame_size = -1;
> @@ -3287,31 +3269,7 @@ aarch64_expand_epilogue (bool for_sibcall)
>        if (need_barrier_p)
>  	emit_insn (gen_stack_tie (stack_pointer_rtx, stack_pointer_rtx));
>  
> -      if (frame_size >= 0x1000000)
> -	{
> -	  rtx op0 = gen_rtx_REG (Pmode, IP0_REGNUM);
> -	  emit_move_insn (op0, GEN_INT (frame_size));
> -	  insn = emit_insn (gen_add2_insn (stack_pointer_rtx, op0));
> -	}
> -      else
> -	{
> -          int hi_ofs = frame_size & 0xfff000;
> -          int lo_ofs = frame_size & 0x000fff;
> -
> -	  if (hi_ofs && lo_ofs)
> -	    {
> -	      insn = emit_insn (gen_add2_insn
> -				(stack_pointer_rtx, GEN_INT (hi_ofs)));
> -	      RTX_FRAME_RELATED_P (insn) = 1;
> -	      frame_size = lo_ofs;
> -	    }
> -	  insn = emit_insn (gen_add2_insn
> -			    (stack_pointer_rtx, GEN_INT (frame_size)));
> -	}
> -
> -      /* Reset the CFA to be SP + 0.  */
> -      add_reg_note (insn, REG_CFA_DEF_CFA, stack_pointer_rtx);
> -      RTX_FRAME_RELATED_P (insn) = 1;
> +      aarch64_add_constant (Pmode, SP_REGNUM, IP0_REGNUM, frame_size);
>      }
>  
>    /* Stack adjustment for exception handler.  */
>