Re: [PATCH 2/6] Andes nds32: machine description of nds32 porting (1).

[Chung-Ju Wu <jasonwucj at gmail dot com>]

On 9/29/13 7:25 PM, Richard Sandiford wrote:
> Chung-Ju Wu <jasonwucj@gmail.com> writes:
> 
>> +  /* We need to provide a customized rtx which contains
>> +     necessary information for data analysis,
>> +     so we create a parallel rtx like this:
>> +     (parallel [(unspec [(reg: Rb)
>> +                         (reg: Re)
>> +                         (const_int En4)] UNSPEC_STACK_PUSH_MULTIPLE)
>> +                (use (reg:SI Rb))
>> +                (use (reg:SI Re))
>> +                (set (mem (plus (reg:SI SP_REGNUM) (const_int -4)))
>> +                     (reg:SI LP_REGNUM))
>> +                (set (mem (plus (reg:SI SP_REGNUM) (const_int -8)))
>> +                     (reg:SI GP_REGNUM))
>> +                (set (mem (plus (reg:SI SP_REGNUM) (const_int -12)))
>> +                     (reg:SI FP_REGNUM))
>> +                (set (mem (plus (reg:SI SP_REGNUM) (const_int -16)))
>> +                     (reg:SI Re))
>> +                ...
>> +                (set (mem (plus (reg:SI SP_REGNUM) (const_int -28)))
>> +                     (reg:SI Rb+1))
>> +                (set (mem (plus (reg:SI SP_REGNUM) (const_int -32)))
>> +                     (reg:SI Rb))
>> +                (set (reg:SI SP_REGNUM)
>> +                     (plus (reg:SI SP_REGNUM) (const_int -32)))]) */
> 
> The "use"s here should be redundant, since the "set"s say that the
> registers are used.  Same comment for:
> 
>> +  /* We need to provide a customized rtx which contains
>> +     necessary information for data analysis,
>> +     so we create a parallel rtx like this:
>> +     (NOTE: We have to clearly claim that we are using/clobbering Rb and Re,
>> +            otherwise it may be renamed by optimization.)
>> +     (parallel [(unspec [(reg: Rb)
>> +                         (reg: Re)
>> +                         (const_int En4)] UNSPEC_STACK_POP_MULTIPLE)
>> +                (use (reg:SI Rb))
>> +                (use (reg:SI Re))
>> +                (clobber (reg:SI Rb))
>> +                (clobber (reg:SI Re))
>> +                (set (reg:SI Rb)
>> +                     (mem (plus (reg:SI SP_REGNUM) (const_int 0))))
>> +                (set (reg:SI Rb+1)
>> +                     (mem (plus (reg:SI SP_REGNUM) (const_int 4))))
>> +                ...
>> +                (set (reg:SI Re)
>> +                     (mem (plus (reg:SI SP_REGNUM) (const_int 16))))
>> +                (set (reg:SI FP_REGNUM)
>> +                     (mem (plus (reg:SI SP_REGNUM) (const_int 20))))
>> +                (set (reg:SI GP_REGNUM)
>> +                     (mem (plus (reg:SI SP_REGNUM) (const_int 24))))
>> +                (set (reg:SI LP_REGNUM)
>> +                     (mem (plus (reg:SI SP_REGNUM) (const_int 28))))
>> +                (set (reg:SI SP_REGNUM)
>> +                     (plus (reg:SI SP_REGNUM) (const_int 32)))]) */
> 
> the "use"s and "clobber"s here.  As far as the note goes, adding clobbers
> isn't guaranteed to stop renaming, and it's not really valid for an rtx
> to assign to the same register twice.
> 
> The best way of preventing renaming is to have a match_parallel that
> checks every "set", a bit like you already did for the load/store multiple.
> That should make the "unspec" unnecessary too.
> 

Now we remove all "use"s and "clobber"s from parallel rtx and
use predicate function to check stack push/pop operation.
Furthermore, once I remove unspec rtx as you suggested,
I notice gcc is aware of the def-use dependency and it wouldn't
perform unexpected register renaming.  So I think it was my
faulty design of placing unspec/use/clobber within parallel rtx.

Thanks for the comment.  The parallel rtx for stack push/pop now
looks much better than my original design. :)

>> +/* Function for nds32_merge_decl_attributes() and nds32_insert_attributes()
>> +   to check if there are any conflict isr-specific attributes being set.
>> +   We need to check:
>> +     1. Only 'save_all' or 'partial_save' in the attributes.
>> +     2. Only 'nested', 'not_nested', or 'nested_ready' in the attributes.
>> +     3. Only 'interrupt', 'exception', or 'reset' in the attributes.  */
>> +static void
>> +nds32_check_isr_attrs_conflict (const char *func_name, tree func_attrs)
>> +{
>> +  int save_all_p, partial_save_p;
>> +  int nested_p, not_nested_p, nested_ready_p;
>> +  int intr_p, excp_p, reset_p;
>> +
>> +  /* Initialize variables.  */
>> +  save_all_p = partial_save_p = 0;
>> +  nested_p = not_nested_p = nested_ready_p = 0;
>> +  intr_p = excp_p = reset_p = 0;
>> +
>> +  /* We must check at MOST one attribute to set save-reg.  */
>> +  if (lookup_attribute ("save_all", func_attrs))
>> +    save_all_p = 1;
>> +  if (lookup_attribute ("partial_save", func_attrs))
>> +    partial_save_p = 1;
>> +
>> +  if ((save_all_p + partial_save_p) > 1)
>> +    error ("multiple save reg attributes to function %qs", func_name);
>> +
>> +  /* We must check at MOST one attribute to set nested-type.  */
>> +  if (lookup_attribute ("nested", func_attrs))
>> +    nested_p = 1;
>> +  if (lookup_attribute ("not_nested", func_attrs))
>> +    not_nested_p = 1;
>> +  if (lookup_attribute ("nested_ready", func_attrs))
>> +    nested_ready_p = 1;
>> +
>> +  if ((nested_p + not_nested_p + nested_ready_p) > 1)
>> +    error ("multiple nested types attributes to function %qs", func_name);
>> +
>> +  /* We must check at MOST one attribute to
>> +     set interrupt/exception/reset.  */
>> +  if (lookup_attribute ("interrupt", func_attrs))
>> +    intr_p = 1;
>> +  if (lookup_attribute ("exception", func_attrs))
>> +    excp_p = 1;
>> +  if (lookup_attribute ("reset", func_attrs))
>> +    reset_p = 1;
>> +
>> +  if ((intr_p + excp_p + reset_p) > 1)
>> +    error ("multiple interrupt attributes to function %qs", func_name);
>> +}
> 
> It's better to use "%qD" directly on the FUNCTION_DECL, since that copes
> better with things like mangling.
> 

Fix accordingly.

>> +  /* If fp$, $gp, $lp, and all callee-save registers are NOT required
>> +     to be saved, we don't have to create multiple pop instruction.
>> +     Otherwise, a multiple pop instruction is needed.  */
> 
> "fp$" -> "$fp"
> 

Fix accordingly.



> Looks good otherwise, thanks.
> 
> Richard
> 

A revised-2 patch for nds32.c and nds32.h is attached.
Thanks for the comments! :)

Best regards,
jasonwucj

Attachment: 2-nds32-backend-md-part1.v3.revised-2.patch
Description: Text document