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.
