Re: [Patch,Microblaze]: Added Break Handler Support

[Michael Eager <eager@eagercon.com>]

On 05/13/14 02:14, Ajit Kumar Agarwal wrote:
> Hello Michael:
>
> The following patch is to handle Software and Hardware breaks in Microblaze Architecture.
> Deja GNU testcase does not have any regressions and the testcase attached passes through.
> Review comments are incorporated.
>
> Okay for trunk?

Just saying OK would only be appropriate if you had write access to the repository.

> Added Break Handler support to incorporate the hardware and software break. The Break Handler routine
> will be generating the rtbd instruction. At the call point where the software breaks are generated with
> the instruction brki with register operand as r16.
>
> 2014-05-13 Ajit Agarwal <ajitkum@xilinx.com>
>
> * config/microblaze/microblaze.c
>     (microblaze_break_function_p,microblaze_is_break_handler) : New
>
> * config/microblaze/microblaze.h (BREAK_HANDLER_NAME) : New macro
>
> * config/microblaze/microblaze.md :
>    Extended support for generation of brki instruction and rtbd instruction.

A better ChangeLog entry is
* config/microblaze/microblaze.md (*<optab>,<optab>_internal):
     Add microblaze_is_break_handler () test.

Give specifics, naming functions, rather than making general comments.
As the ChangeLog standard says:
   It’s important to name the changed function or variable in full.
   Don’t abbreviate function or variable names, and don’t combine them.
   Subsequent maintainers will often search for a function name to find
   all the change log entries that pertain to it; if you abbreviate the
   name, they won’t find it when they search.

Mention each place where there are changes.  There should be a ChangeLog
entry for each non-trivial change.

Your patch made four significant changes to microblaze.md.
There appear to be several changes in microblaze.c, not just the definition
of the new functions as shown in your entry.


> * config/microblaze/microblaze-protos.h
>     (microblaze_break_function_p,microblaze_is_break_handler) : New Declaration.
>
> * testsuite/gcc.target/microblaze/others/break_handler.c : New.

Thanks for the test case.

As mentioned previously, add documentation for _break_handler.

> diff --git a/gcc/config/microblaze/microblaze-protos.h b/gcc/config/microblaze/microblaze-protos.h
> index b03e9e1..f3cc099 100644
> --- a/gcc/config/microblaze/microblaze-protos.h
> +++ b/gcc/config/microblaze/microblaze-protos.h

Please include the patch only once, not both inline and again as an
attachment.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077