This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi, Joseph,

Sorry for the late revised patch.
We have completed all of it based on your review comments.

On 7/10/13 7:20 AM, Joseph S. Myers wrote:
On Mon, 8 Jul 2013, Chung-Ju Wu wrote:

+/* ------------------------------------------------------------------------ */
+
+/*++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++*
+ *|                                                                        |*
+ *| This file is divided into six parts:                                   |*
+ *|                                                                        |*
+ *|   PART 1: Auxiliary static function and variable declarations.         |*
+ *|                                                                        |*
+ *|   PART 2: Target hooks static function and variable declarations.      |*
+ *|                                                                        |*
+ *|   PART 3: Initialize target hooks structure and definitions.           |*
+ *|                                                                        |*
+ *|   PART 4: Implement auxiliary static function definitions,             |*
+ *|           the prototype is in nds32.c.                                 |*
+ *|                                                                        |*
+ *|   PART 5: Implement target hook stuff definitions,                     |*
+ *|           the prototype is in nds32.c.                                 |*
+ *|                                                                        |*
+ *|   PART 6: Implement extern function definitions,                       |*
+ *|           the prototype is in nds32-protos.h.                          |*
+ *|                                                                        |*
+ *++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++*/

Fancy comment formatting like this is not the norm in GCC.  Comments
should just look like

/* Comment text.
    Subsequent lines lined up like this.

    End of comment.  */


We removed the fancy comment formatting as you suggested.
However, for the comments like:

/* ===================== */
/* Title of section */
/* ===================== */

/* Title of subsection 1 */

/* Title of subsection 2 */

I wish I can keep such kinds of comment formatting so that
it would be easier to distinguish the levels between sections
and subsections against the chapter 17 in GCC Internals documentation.

Is it ok to keep such formatting? :)

I also advise topologically sorting static functions and variables and
putting the target hook structure initialization at the end of the file,
to minimize the number of static declarations needed separate from the
definitions (they should generally be needed only for functions called
recursively).


Thanks for the suggestion.  We made such changes accordingly.

+/* Define intrinsic register names.
+   Please refer to nds32_intrinsic.h file, the index is corresponding to
+   'enum nds32_intrinsic_registers' data type values.
+   NOTE that the base value starting from 1024.  */
+static const char* nds32_intrinsic_register_names[] =

I suspect the array here can itself be made const.


I am declaring an array in which every element is "const char *" type.
It is different to the declaration:
"static char* const nds32_intrinsic_register_names[] ="

+/* 17.8 Register Classes */

Please avoid all comments referring to section numbers in particular
versions of the manual; there's no way those comments are ever going to
get updated when sections are added, removed, renamed or reordered.


Fixed it accordingly.

+  /* Prepare jmptbl section and symbol name.  */
+  sprintf (section_name, ".nds32_jmptbl.%02d", vector_id);
+  sprintf (symbol_name, "_nds32_jmptbl_%02d", vector_id);

Anywhere you use sprintf, please use snprintf instead (with the size
argument generally being sizeof (buffer_variable), as good coding practice
to avoid buffer overruns if there was a mistake in the reasoning that the
buffer size was big enough.


Fixed it accordingly.

+  /* Currently, we don't support PIC code generation yet.  */
+  if (flag_pic)
+    error ("not support -fpic");

Something not supported yet sounds like a case for sorry () instead of
error ().


Fixed it accordingly.

+/* For the following option pairs:
+     1. (-mbig-endian, -mlittle-endian)
+     2. (-mreduced-regs, -mfull-regs)
+   We reject them to appear in the command line.  */
+#define CC1_SPEC                                                              \
+  " %{mbig-endian:"                                                           \
+  "   %{mlittle-endian:%emay not use both -mbig-endian and -mlittle-endian}}" \
+  " %{mreduced-regs:"                                                         \
+  "   %{mfull-regs:%emay not use both -mreduced-regs and -mfull-regs}}"

If you put these options in Negative() loops in the .opt file, then you
can have the last option on the command line winning (with only the last
option being visible to specs), which is the usual practice, instead of
having such errors.


Thanks so much.  The approach you suggested is definitely much better.
We implemented it in nds32.opt file.

+#define LINK_SPEC \
+  " %{v} %{mbig-endian:-EB} %{mlittle-endian:-EL}"

Do you really need %{v} here?  Such things shouldn't generally be
architecture-specific.


Oops.. thanks to point out this mistake.
Removed it accordingly.


Thanks for the review comments.
A revised patch is provided and here is a summary:

   1. Remove fancy comments formatting.
   2. Sort static functions and variables and put the target hook
      structure initialization at the end of the file.
   3. Avoid all comments referring to section numbers.
   4. Use snprintf () instead of sprinf ().
   5. Use sorry () instead of error () for unimplemented functionality.
   6. Do not check command options error in nds32.h.
      Use Negative() in nds32.opt instead.
   7. Remove %{v} from LINK_SPEC.
   8. Add OPTION_DEFAULT_SPECS for -march option.
   9. Enable/disable corresponding flags internally for -march= option.
  10. Add MULTILIB_DEFAULTS for multilib endianness.


gcc/
2013-07-24  Chung-Ju Wu  <jasonwucj@gmail.com>
	    Shiva Chen  <shiva0217@gmail.com>

	* config/nds32/nds32.c: New file.
	* config/nds32/nds32.h: New file.


Best regards,
jasonwucj

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]