On Thu, 2011-11-10 at 13:44 +0000, Richard Earnshaw wrote:
> On 28/09/11 17:15, Sameera Deshpande wrote:
> > Hi!
> > 
> > This patch generates Thumb2 epilogues in RTL form.
> > 
> > The work involves defining new functions, predicates and patterns along with
> > few changes in existing code:
> > * The load_multiple_operation predicate was found to be too restrictive for
> > integer loads as it required consecutive destination regs, so this
> > restriction was lifted.
> > * Variations of load_multiple_operation were required to handle cases 
> >    - where SP must be the base register 
> >    - where FP values were being loaded (which do require consecutive
> > destination registers)
> >    - where PC can be in register-list (which requires return pattern along
> > with register loads).
> >   Hence, the common code was factored out into a new function in arm.c and
> > parameterised to show 
> >    - whether consecutive destination regs are needed
> >    - the data type being loaded 
> >    - whether the base register has to be SP
> >    - whether PC is in register-list
> > 
> > The patch is tested with arm-eabi with no regressions.
> > 
> > ChangeLog:
> > 
> > 2011-09-28  Ian Bolton         <ian.bolton@arm.com>
> >             Sameera Deshpande  <sameera.deshpande@arm.com>
> >            
> >        * config/arm/arm-protos.h (load_multiple_operation_p): New
> > declaration.
> >          (thumb2_expand_epilogue): Likewise.
> >          (thumb2_output_return): Likewise
> >          (thumb2_expand_return): Likewise.
> >          (thumb_unexpanded_epilogue): Rename to... 
> >          (thumb1_unexpanded_epilogue): ...this 
> >        * config/arm/arm.c (load_multiple_operation_p): New function. 
> >          (thumb2_emit_multi_reg_pop): Likewise.
> >          (thumb2_emit_vfp_multi_reg_pop): Likewise.
> >          (thumb2_expand_return): Likewise. 
> >          (thumb2_expand_epilogue): Likewise. 
> >          (thumb2_output_return): Likewise
> >          (thumb_unexpanded_epilogue): Rename to...
> >          ( thumb1_unexpanded_epilogue): ...this
> >        * config/arm/arm.md (pop_multiple_with_stack_update): New pattern. 
> >          (pop_multiple_with_stack_update_and_return): Likewise.
> >          (thumb2_ldr_with_return): Likewise.
> >          (floating_point_pop_multiple_with_stack_update): Likewise.
> >          (return): Update condition and code for pattern.
> >          (arm_return): Likewise.
> >          (epilogue_insns): Likewise.
> >        * config/arm/predicates.md (load_multiple_operation): Update
> > predicate.
> >          (load_multiple_operation_stack_and_return): New predicate. 
> >          (load_multiple_operation_stack): Likewise.
> >          (load_multiple_operation_stack_fp): Likewise.
> >        * config/arm/thumb2.md (thumb2_return): Remove.
> >          (thumb2_rtl_epilogue_return): New pattern.
> > 
> > 
> > - Thanks and regards,
> >   Sameera D.
> > 
> > 
> > thumb2_rtl_epilogue_complete-27Sept.patch
> > 
> 
> +  if (GET_CODE (SET_SRC (elt = XVECEXP (op, 0, offset_adj))) == PLUS)
> 
> It's generally best not to use assignments within conditionals unless
> there is a strong reason otherwise (that normally implies something like
> being deep within a condition test where you only want to update the
> variable if some pre-conditions are true and that can't be easily
> factored out).
> 
> +                  != (unsigned int) (first_dest_regno + regs_per_val *
> (i - base))))
> 
> Line length (split the line just before the '+' operator.
> 
> +  /* now show EVERY reg that will be restored, using a SET for each.  */
> 
> Capital letter at start of sentence.  Why is EVERY in caps?
> 
> +  saved_regs_mask = offsets->saved_regs_mask;
> +  for (i = 0, num_regs = 0; i <= LAST_ARM_REGNUM; i++)
> 
> blank line before the for loop.
> 
> +      /* It's illegal to do a pop for only one reg, so generate an ldr.  */
> 
> GCC coding standards suggest avoiding the use of 'illegal'.  Suggest
> changing that to 'Pop can only be used for more than one reg; so...'
> 
> +                reg_names[REGNO (XEXP (XVECEXP (operands[0], 0, 2),
> 0))]);
> +
> +    /* Skip over the first two elements and the one we just generated.
>  */
> +    for (i = 3; i < (num_saves); i++)
> +      {
> +        strcat (pattern, \", %|\");
> 
> +        strcat (pattern,
> 
> +                reg_names[REGNO (XEXP (XVECEXP (operands[0], 0, i),
> 0))]);
> +      }
> +
> +    strcat (pattern, \"}\");
> +    output_asm_insn (pattern, operands);
> +
> 
> +    return \"\";
> +  }
> +  "
> 
> +  [(set_attr "type" "load4")]
> 
> There's a lot of trailing white space here.  Please remove.
> 
> +(define_insn "*thumb2_ldr_with_return"
> +  [(return)
> +   (set (reg:SI PC_REGNUM)
> +        (mem:SI (post_inc:SI (match_operand:SI 0 "s_register_operand"
> "k"))))]
> +  "TARGET_THUMB2"
> +  "ldr%?\t%|pc, [%0], #4"
> +  [(set_attr "type" "load1")
> +   (set_attr "predicable" "yes")]
> +)
> +
> 
> This pattern doesn't seem to be used.  What's its purpose?
> 
> +    static const struct { const char *const name; } table[]
> +                  = { {\"d0\"}, {\"d1\"}, {\"d2\"}, {\"d3\"},
> 
> I'm not keen on having this table.  Generally the register names should
> be configurable depending on the assembler flavour and this patch
> defeats that.  Is there any way to rewrite this code so that it can use
> the standard operand methods for generating register names?
> 
> In summary, this is mostly OK, apart from the last two items.
> 
> R.

Richard,

Please find attached the reworked patch.

--