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: Assembly output optimisations (was: PR 51094 - fprint_w() in output_addr_const() reinstated)


On Tue, 7 Aug 2012, Dimitrios Apostolou wrote:
> Thanks Andreas, hp, Mike, for your comments. Mike I'd appreciate if you
> elaborated on how to speed-up sprint_uw_rev(), I don't think I understood what
> you have in mind.

I just commented on comments and just above the nit-level;
formatting and contents.  Still, I believe such nits affects
reviewer interest.

Formatting: two spaces after punctuation at end of sentence, see
other comments.  You got it right in most places.  Some of the
comments you changed were actually wrong *before*, some of those
you actually corrected.

I can't quote the non-inlined patch, so I'll copy-paste some
lines, adding quote-style:

> +/* Return a statically allocated string with the decimal representation of
> +   VALUE. String IS NOT null-terminated.  */

Write as:
+/* Return a statically allocated string with the decimal representation of
+   VALUE.  String IS NOT null-terminated.  */

Avoid comments that look like commented out code.  Add a word or
two to avoid that.

> +      /* Emit the .loc directive understood by GNU as. Equivalent: */
> +      /* printf ("\t.loc %u %u 0 is_stmt %u discriminator %u",
> +               file_num, line, is_stmt, discriminator); */

Write as:
+      /* Emit the .loc directive understood by GNU as.  The following is
+         equivalent to printf ("\t.loc %u %u 0 is_stmt %u discriminator %u",
+         file_num, line, file_num, line, is_stmt, discriminator);  */

Most of the new comments in default_elf_asm_output_ascii are
wrongly formatted.  Most are missing end-of-sentence punctuation
and two spaces.  Some look odd; complete the sentences or add
ellipsis to continue in the next comment.  There's also wrong
terminology both in comments and code, in several places: a '\0'
is NIL, not NULL.  Better write as \0 for clarity in comments.

> +         /* If short enough */
> +         if (((unsigned long) (next_null - s) < ELF_STRING_LIMIT)
> +             /* and if it starts with NULL and it is only a
> +                single NULL (empty string) */
> +             && ((*s != '\0') || (s == next_null)))
> +           /* then output as .string */
> +           s = default_elf_asm_output_limited_string (f, s);
> +         else
> +           /* long string or many NULLs, output as .ascii */

Write as (note also the s/next_null/next_nil/):
+         /* If short enough...  */
+         if (((unsigned long) (next_nil - s) < ELF_STRING_LIMIT)
+             /* ... and if it starts with \0 and it is only
+                single \0 (an empty string) ...  */
+             && ((*s != '\0') || (s == next_null)))
+           /* ... then output as .string.  */
+           s = default_elf_asm_output_limited_string (f, s);
+         else
+           /* A long string or many \0, output as .ascii.  */

Contents: I missed a note on relative speedups.  You need to
guard the changes, or else a valid future cleanup could be to
simplify the code, using library functions, possibly slowing it
down.  There's just the word "fast" added in a comment,
reminding of self-adhesive go-fast-stripes for cars and computer
cases. :)

> /* Write a signed HOST_WIDE_INT as decimal to a file, fast.  */

But *how* much faster?  Something like:
/* Write a signed HOST_WIDE_INT as decimal to a file.  At some
   time, this was X times faster than fprintf ("%lld", l);  */

Right, not the most important review bits.  Still, you asked for
elaboration.  Oh right, I see that was just for Mike's comments.
Well, anyway, since my comments still apply. :P

brgds, H-P


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