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

[Sameera Deshpande <sameera.deshpande@arm.com>]

Hi Richard,

thanks for your comments.

-- 



> +  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.

Removed white spaces in reworked patch
http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01009.html

> 
> +(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?

This pattern is generated from thumb2_expand_return in 

+      if (num_regs == 1)
+        {
+          rtx par = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (2));
+          rtx reg = gen_rtx_REG (SImode, PC_REGNUM);
+          rtx addr = gen_rtx_MEM (SImode,
+                                  gen_rtx_POST_INC (SImode,
+
stack_pointer_rtx));
+          set_mem_alias_set (addr, get_frame_alias_set ());
+          XVECEXP (par, 0, 0) = ret_rtx;
+          XVECEXP (par, 0, 1) = gen_rtx_SET (SImode, reg, addr);
+          RTX_FRAME_RELATED_P (par) = 1;
+          emit_jump_insn (par);
+        }

> 
> +    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?

The updated patch was resent after comments from Ramana and Paul which
eliminates this table.

http://gcc.gnu.org/ml/gcc-patches/2011-11/msg01009.html

I will take care of other formatting issues and will resend the patch.

> 
> In summary, this is mostly OK, apart from the last two items.
> 
> R.

- Thanks and regards,
  Sameera D.