[RFA/ARM][Patch 01/02]: Thumb2 epilogue in RTL
Sameera Deshpande
sameera.deshpande@arm.com
Thu Nov 10 19:07:00 GMT 2011
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.
--
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thumb2_rtl_epilogue_complete-10Nov.patch
Type: text/x-patch
Size: 29591 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20111110/b4e2493e/attachment.bin>
More information about the Gcc-patches
mailing list