Chung-Ju Wu <jasonw...@gmail.com> writes:
>>> +/* Permitting tail calls. */
>>> +
>>> +static bool
>>> +nds32_warn_func_return (tree decl)
>>> +{
>>> + /* Naked functions are implemented entirely in assembly, including the
>>> + return sequence, so suppress warnings about this. */
>>> + return !nds32_naked_function_p (decl);
>>> +}
>>
>> The comment above the function doesn't seem to match the function.
>>
>
> The TARGET_WARN_FUNC_RETURN is located at "17.10.13 Permitting tail calls"
> in GCC Internal 4.9.0 pre-release.
Hmm, good point. That seems kind-of misplaced.
> I should have added comment to describe
> nds32_warn_func_return() to avoid such confusion. Like this:
>
> /* Permitting tail calls. */
>
> /* Determine whether we need to enable warning for function return check.
> */
> static bool
> nds32_warn_func_return (tree decl)
Look's good, thanks.
>>> + /* If the function is 'naked', we do not have to generate
>>> + epilogue code fragment BUT 'ret' instruction. */
>>> + if (cfun->machine->naked_p)
>>> + {
>>> + /* Generate return instruction by using unspec_func_return pattern.
>>> + Make sure this instruction is after gen_blockage().
>>> + NOTE that $lp will become 'live'
>>> + after this instruction has been emitted. */
>>> + emit_insn (gen_unspec_func_return ());
>>> + return;
>>> + }
>>
>> FWIW, this is different from AVR (for one), which explicitly doesn't emit a
>> return instruction. One of the uses for AVR is to chain code together
>> into an .init-like section.
>>
>
> Yes.
> Personally I prefer not to generate 'ret' instruction for a naked function.
> Programmers have responsibility to take all the jobs and do everything
> they need with c code, attribute, and inline asm.
>
> But some of our existing customers also expect that a naked function
> is supposed to imply "no push/pop" only. Eventually we take their
> requirement into account, having 'ret' produced by compiler automatically.
That's certainly OK with me FWIW. I don't think there's any realistic
hope of having the attribute behave the same way everywhere, because
it's already been used in incompatible ways on different targets.
>> Redundant brackets around the ==. There are a few other cases in the
>> patch too, although most of it seems OK.
>>
>
> Sorry I didn't see such requirement in GNU coding standards or
> GCC coding convention. If it is not defined in coding convention,
> may I keep these redundant brackets for readability? :)
Now you mention it, I can't find it written down anywhere either.
Maybe it's one of those unwritten things that I've been taking
for granted, or maybe I just imagined it :-)
So I can't push back on this.
> + /* 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.
> +/* 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.
> + /* 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"
Looks good otherwise, thanks.
Richard
