Re: PATCH: new port: Moxie

[Anthony Green <green at moxielogic dot com>]

Richard,

 Thanks for spending time to review this patch.  A new version of the 
port is attached below.  Comments inlined below..

Richard Henderson wrote:
> > +(define_insn "addsi3"
> > +  [(set (match_operand:SI 0 "register_operand" "=r,r,r")
> > +         (plus:SI
> > +          (match_operand:SI 1 "register_operand" "0,0,0")
> > +          (match_operand:SI 2 "general_operand" "I,N,r")))]
>
> Use nonmemory_operand or, better, a new predicate that exactly
> matches {reg, -255..255}.  Same for all other arithmetic insns
> where memories aren't accepted.
Ok.

> > +(define_expand "cbranchsi4"
> > +  [(set (reg:CC CC_REG)
> > +        (compare:CC
> > +         (match_operand:SI 1 "general_operand" "")
> > +         (match_operand:SI 2 "general_operand" "")))
> > +        (set (pc)
> > +          (if_then_else (match_operator:CC 0 "comparison_operator"
> > +                            [(reg:CC CC_REG) (const_int 0)])
> > +                       (label_ref (match_operand 3 "" ""))
> > +                       (pc)))]
>
> Bad indentation.
Fixed.


> > +(define_code_attr CC [(ne "ne ") (eq "eq ") (lt "lt ") (ltu "ltu") 
> > +                     (gt "gt ") (gtu "gtu") (ge "ge ") (le "le ") 
> > +                     (geu "geu") (leu "leu") ])
>
> Spurious spaces.  The user of <CC> already has a trailing tab.
Fixed.


> > +(define_insn "*call"
> > +  [(call (mem:QI (match_operand:SI
> > +                 0 "immediate_operand" "i"))
> > +        (match_operand 1 "" ""))]
> > +  ""
> > +  "jsra   %0")
> > +
> > +(define_insn "*call_indirect"
> > +  [(call (mem:QI (match_operand:SI
> > +                 0 "register_operand" "r"))
> > +        (match_operand 1 "" ""))]
> > +  ""
> > +  "jsr    %0")
>
> These should be alternates of the same pattern.  If your assembler
> can't actually handle "jsra 3" you should use "s" not "i" to allow
> only symbolic constants.
Fixed (and it can handle "jsra 3").


> > +void
> > +moxie_expand_epilogue (void)
> > +{
> > +  int regno;
> > +  rtx insn;
> > +
> > +  if (cfun->machine->callee_saved_reg_size != 0)
> > +    {
> > +      insn = +       emit_insn (gen_movsi (gen_rtx_REG (Pmode, 
> > MOXIE_R12), +                             GEN_INT 
> > (cfun->machine->callee_saved_reg_size)));
> > +      RTX_FRAME_RELATED_P (insn) = 1;
> > +      insn = emit_insn (gen_movsi (stack_pointer_rtx, 
> > hard_frame_pointer_rtx));
> > +      insn = +       emit_insn (gen_subsi3 (stack_pointer_rtx, 
> > +                              stack_pointer_rtx, 
> > +                              gen_rtx_REG (Pmode, MOXIE_R12)));
> > +      RTX_FRAME_RELATED_P (insn) = 1;
> > +
> > +      for (regno = FIRST_PSEUDO_REGISTER; regno > 0; --regno)
> > +       if (df_regs_ever_live_p (regno) && (! call_used_regs[regno]))
> > +         {
> > +           insn = emit_insn (gen_movsi_pop (gen_rtx_REG (Pmode, 
> > regno)));
> > +           RTX_FRAME_RELATED_P (insn) = 1;
> > +         }
> > +    }
> > +
> > +  insn = emit_jump_insn (gen_returner ());
> > +  RTX_FRAME_RELATED_P (insn) = 1;
> > +}
>
> Um... This epilogue doesn't look interrupt safe.  You deallocate
> the entire stack frame then allocate back to the beginning of the
> saved registers.

Good catch!

> Also, now that my epilogue unwinding code is in, this is actively
> wrong and will almost certainly abort.  New version should look like
Fixed.


> void
> moxie_expand_epilogue ()
> {
>   int regno;
>   rtx insn, reg, cfa_restores = NULL;
>
>   if (cfun->machine->callee_saved_reg_size != 0)
>     {
>       reg = gen_rtx_REG (Pmode, MOXIE_R12);
>       emit_move_insn (reg,
>         GEN_INT (-cfun->machine->callee_saved_reg_size);
>       emit_insn (gen_addsi3 (reg, reg, hard_frame_pointer_rtx));
>       insn = emit_move_insn (stack_pointer_rtx, reg);
>       RTX_FRAME_RELATED_P (insn) = 1;
>       add_reg_note (insn, REG_CFA_DEF_CFA,
>                     plus_constant (stack_pointer_rtx,
>                                 cfun->machine->callee_saved_reg_size));
>
>       for (regno = FIRST_PSEUDO_REGISTER; regno-- > 0; )
>         if (!fixed_regs && !call_used_regs[regno]
>             && df_regs_ever_live_p (regno))
>           {
>             reg = gen_rtx_REG (Pmode, regno);
>             insn = emit_insn (gen_movsi_pop (reg));
>             RTX_FRAME_RELATED_P (insn) = 1;
>             add_reg_note (insn, REG_CFA_ADJUST_CFA,
>                           gen_rtx_SET (VOIDmode, stack_pointer_rtx,
>                                        plus_constant (stack_pointer_rtx,
>                                                       UNITS_PER_WORD)));
>             add_reg_note (insn, REG_CFA_RESTORE, reg);
>           }
>     }
>
>   emit_jump_insn (gen_returner ());
> }
>
>
>
> r~


Thanks again.

AG

Attachment: moxie-gcc-7.patch.gz
Description: application/gzip