Re: RFA: Add support for Renesas RX architectiure to GCC

[Richard Henderson <rth at redhat dot com>]

// Continuing review with rx.[ch]

>             /* FIXME: Adjust this value ?  */
>             if (val < 0 || val > 32767)
>               return false;

Fixme?  And this doesn't seem to agree with the chip documentation.

>         case MULT:
>           /* FIXME: Should we allow ASHIFT here as well ?  */

No, you shouldn't.

>       /* Only allow REG+INT addressing.  */
>       base = XEXP (mem, 0);
>       index = XEXP (mem, 1);
>
>       return (RX_REG_P (base)  && CONST_INT_P (index))
>         ||   (RX_REG_P (index) && CONST_INT_P (base));

You don't need to allow (plus (const_int) (reg)), either here or in 
rx_is_legitimate_address.  Or in rx_print_operand_address.  These are 
non-canonical.

>     default:
>       debug_rtx (src);
>       gcc_unreachable ();

debugging stuff?

>   /* If the size is not known it cannot be passed in registers.  */
>   if (size < 1)
>     return NULL_RTX;

You'll never see this, since this case is handed by pass_by_reference.

> rx_function_arg_boundary (enum machine_mode mode,
>                           const_tree type ATTRIBUTE_UNUSED)
> {
>   return get_mode_alignment (mode);

If you're rounding up the size of the argument to word-size, why isn't 
the alignment word sized as well?

>   /* Never tailcall something for which we have no decl.  */
>   if (decl == NULL)
>     return false;

Why?  It would appear that at least r5 and r14 are free.

>   /* All normal functions are OK.  */
>   return ! is_fast_interrupt_func (decl)
>     &&   ! is_exception_func (decl)
>     &&   ! is_naked_func (decl);

I'm pretty sure you don't want to tail call *from* an interrupt function 
either.

>           fixed_regs[15] = call_used_regs[15] = 1;

Um, this is your struct_value_regnum.  How can this possibly work?

>             insn = emit_insn (gen_stack_push (GEN_INT (reg)));
>             RTX_FRAME_RELATED_P (insn) = 1;

Marking the insn as frame-related can't work when you're using integers 
to represent registers.  Which I've already mentioned is a mistake.

>   /* If needed, set up the frame pointer.  */
>   if (frame_pointer_needed)
>     {
>       if (frame_size)
>         insn = emit_insn (gen_addsi3 (frame_pointer_rtx, stack_pointer_rtx,
>                                       GEN_INT (- (HOST_WIDE_INT) frame_size)));
>       else
>         insn = emit_move_insn (frame_pointer_rtx, stack_pointer_rtx);
>
>       RTX_FRAME_RELATED_P (insn) = 1;
>     }
...
>   else if (frame_size)
>     {
>       if (frame_pointer_needed)
>         insn = emit_move_insn (stack_pointer_rtx, frame_pointer_rtx);

Surely this second hunk isn't needed?

>      In big-endian-data-mode however instructions are read into the CPU
>      4 bytes at a time.  These bytes are then swapped around before being
>      passed to the decoder.  So...we must partition our trampoline into
>      4 byte packets and swap these packets around so that the instruction
>      reader will reverse the process.  But, in order to avoid splitting
>      the 32-bit constants across these packet boundaries, (making inserting
>      them into the constructed trampoline very difficult) we have to pad the
>      instruction sequence with NOP insns.  ie:
>
>            nop
>            nop
>            mov.l        #<...>, r8
>            nop
>            nop
>            mov.l        #<...>, r9
>            jmp          r9
>            nop
>            nop             */

Wouldn't it be simpler to

>    2 0000 FD 6A 19                		mvfc	pc, r9
>    3 0003 ED 98 03                		mov.l	12[r9], r8
>    4 0006 ED 99 04                		mov.l	16[r9], r9
>    5 0009 7F 09                   		jmp	r9
>    6 000b 03                      		nop
>    7 000c EF BE AD DE             		.long	0xdeadbeef
>    8 0010 EF BE AD DE             		.long	0xdeadbeef

Certainly this is 4 bytes smaller than your sequence above.

> rx_trampoline_init (rtx tramp, tree fndecl, rtx chain)
> {
>   if (TARGET_BIG_ENDIAN_DATA)
>     {
>       emit_move_insn (gen_rtx_MEM (SImode, plus_constant (tramp, 4)), chain);
>       emit_move_insn (gen_rtx_MEM (SImode, plus_constant (tramp, 12)), fndecl);
>     }
>   else
>     {
>       emit_move_insn (gen_rtx_MEM (SImode, plus_constant (tramp, 2)), chain);
>       emit_move_insn (gen_rtx_MEM (SImode, plus_constant (tramp, 6 + 2)), fndecl);
>     }
> }

This is not a proper conversion.  Have another look at the other ports.
Also, don't you have a STRICT_ALIGNMENT target?  How is this 
2-byte-aligned offset going to work?


r~