On 01/10/2012 11:55 PM, Jayant R. Sonar wrote:
> PING 9: For reviewing the modified CR16 port.
>
> Hello,
>
> Can some one please review the updated patch and let me know if any more
> changes are required to be done in it?
>
> Rainer had suggested few important changes last time. After making those
> changes, the modified patch was posted at following URL:
> http://gcc.gnu.org/ml/gcc-patches/2011-11/msg02625.html
>
> Reference links to the past discussions about this port are also available
> at the above mentioned URL.
>
> I am hoping that we will be able to see this port in GCC 4.7.
...
> The CR16's programming memory is 2-byte aligned and the least significant
> bit of PC is always zero. The Return Address (RA) register always saves the
> value of PC right shifted by 1(PC >> 1). This conversion seems broken at
> some places during unwinding which results in terminate() function being
> called.
This sounds like a job for __builtin_frob_return_addr. At present we only
have hooks for Sparc's RETURN_ADDR_OFFSET, but that could easily be changed
to perform any target-specific transformation.
Minor errors:
> +#define MUST_SAVE_REGS_P() \
> + (flag_unwind_tables || (flag_exceptions && !UI_SJLJ))
UI_SJLJ is an enumeration constant. You wanted to test the result of
targetm_common.except_unwind_info(). It might be better to fold this
macro into its only user.
That said, this optimization also affects debugging, in that you may
no longer be able to examine variables in stack frames higher up.
E.g. after you've stopped at a breakpoint set on abort(). Do you
really want this at lower optimization levels?
> + /* If -fpic option, data_model == DM_FAR. */
> + if (flag_pic == NEAR_PIC)
> + {
> + data_model = DM_FAR;
> + }
> +
> + /* The only option we want to examine is data model option. */
> + if (cr16_data_model)
> + {
> + if (strcmp (cr16_data_model, "medium") == 0)
> + data_model = DM_DEFAULT;
> + else if (strcmp (cr16_data_model, "near") == 0)
> + data_model = DM_NEAR;
> + else if (strcmp (cr16_data_model, "far") == 0)
> + {
> + if (TARGET_CR16CP)
> + data_model = DM_FAR;
> + else
> + error ("data-model=far not valid for cr16c architecture");
> + }
> + else
> + error ("invalid data model option -mdata-model=%s", cr16_data_model);
> + }
> + else
> + data_model = DM_DEFAULT;
The first IF is ineffective because the second IF always overwrites data_model.
> +(define_insn "push_for_prologue"
...
> +(define_insn "pop_and_popret_return"
I'm not keen on the fact that the integral argument is totally ignored.
E.g. the post-reload pass_stack_adjustments could adjust the amount from
the value you computed in cr16_compute_frame.
> +(define_insn "set_bit<mode>_mem"
> + [(set (match_operand:SHORT 0 "bit_operand" "=m")
...
> +(define_insn "clear_bit<mode>_mem"
> + [(set (match_operand:SHORT 0 "bit_operand" "=m")
"+m"
As far as I'm concerned these problems could even be fixed post-commit.
r~
