Re: [RFA/ARM][Patch 01/02]: Thumb2 epilogue in RTL

[Richard Earnshaw <rearnsha@arm.com>]

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.