[PATCH] sim: tighten up generated decode tables

[PATCH] sim: tighten up generated decode tables

[Mike Frysinger]

The use of /* fall through */ with consective case statements doesn't
really add any value, and when generating large files, can take up a
lot of space.  In the case of cris, it alone adds ~20k, or ~10%.

Also trim the space before the : with case statements.
---
 utils-sim.scm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/utils-sim.scm b/utils-sim.scm
index dca0f0705036..8011bb775933 100644
--- a/utils-sim.scm
+++ b/utils-sim.scm
@@ -684,14 +684,14 @@
 		(obj:name (dtable-entry-value (car rest)))))
       (string-append indent "  case "
 		     (number->string (dtable-entry-index entry))
-		     " : /* fall through */\n"))
+		     ":\n"))
 
      (else
       (let ((consistent-base-insn? (and (equal? APPLICATION 'SID-SIMULATOR)
 					(> (state-base-insn-bitsize)
 					   (insn-length insn)))))
 	(string-append indent "  case "
-		       (number->string (dtable-entry-index entry)) " :"
+		       (number->string (dtable-entry-index entry)) ":"
 		       ;; Compensate for base-insn-size > current-insn-size by
 		       ;; adjusting entire_insn.
 		       ;; Activate this logic only for sid simulators; they are
@@ -825,7 +825,7 @@
     (string-list
      indent "  case "
      (number->string (dtable-entry-index entry))
-     " :\n"
+     ":\n"
 
      (let ((iflds-tracking (/decode-expr-ifield-tracking expr-list))
 	   (indent (string-append indent "    ")))
@@ -914,7 +914,7 @@
   (string-list
    indent "  case "
    (number->string (dtable-entry-index table))
-   " :"
+   ":"
    ; If table is same as next, just emit a "fall through" to cut down on
    ; generated code.
    (if (and (not (null? rest))
@@ -923,7 +923,7 @@
 	    ; Ensure same table.
 	    (eqv? (subdtable-key (dtable-entry-value table))
 		  (subdtable-key (dtable-entry-value (car rest)))))
-       " /* fall through */\n"
+       "\n"
        (string-list
 	"\n"
 	(/gen-decoder-switch switch-num
@@ -1075,7 +1075,7 @@
 		  result))))
 
      ;; ??? Can delete if all cases are present.
-     indent "  default : "
+     indent "  default: "
      (/gen-decode-default-entry invalid-insn fn?) "\n"
      indent "  }\n"
      indent "}\n"
-- 
2.43.0

Re: [PATCH] sim: tighten up generated decode tables

[Jose E. Marchesi]

> The use of /* fall through */ with consective case statements doesn't
> really add any value, and when generating large files, can take up a
> lot of space.  In the case of cris, it alone adds ~20k, or ~10%.

I am a little concern this change may trigger implicit-fallthrough
warnings when compiling the generated code.  Not sure this is a problem
in practice though, since nor binutils nor sim uses
-Wimplicit-fallthrough for building as far as I can see.

> Also trim the space before the : with case statements.
> ---
>  utils-sim.scm | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/utils-sim.scm b/utils-sim.scm
> index dca0f0705036..8011bb775933 100644
> --- a/utils-sim.scm
> +++ b/utils-sim.scm
> @@ -684,14 +684,14 @@
>  		(obj:name (dtable-entry-value (car rest)))))
>        (string-append indent "  case "
>  		     (number->string (dtable-entry-index entry))
> -		     " : /* fall through */\n"))
> +		     ":\n"))
>  
>       (else
>        (let ((consistent-base-insn? (and (equal? APPLICATION 'SID-SIMULATOR)
>  					(> (state-base-insn-bitsize)
>  					   (insn-length insn)))))
>  	(string-append indent "  case "
> -		       (number->string (dtable-entry-index entry)) " :"
> +		       (number->string (dtable-entry-index entry)) ":"
>  		       ;; Compensate for base-insn-size > current-insn-size by
>  		       ;; adjusting entire_insn.
>  		       ;; Activate this logic only for sid simulators; they are
> @@ -825,7 +825,7 @@
>      (string-list
>       indent "  case "
>       (number->string (dtable-entry-index entry))
> -     " :\n"
> +     ":\n"
>  
>       (let ((iflds-tracking (/decode-expr-ifield-tracking expr-list))
>  	   (indent (string-append indent "    ")))
> @@ -914,7 +914,7 @@
>    (string-list
>     indent "  case "
>     (number->string (dtable-entry-index table))
> -   " :"
> +   ":"
>     ; If table is same as next, just emit a "fall through" to cut down on
>     ; generated code.
>     (if (and (not (null? rest))
> @@ -923,7 +923,7 @@
>  	    ; Ensure same table.
>  	    (eqv? (subdtable-key (dtable-entry-value table))
>  		  (subdtable-key (dtable-entry-value (car rest)))))
> -       " /* fall through */\n"
> +       "\n"
>         (string-list
>  	"\n"
>  	(/gen-decoder-switch switch-num
> @@ -1075,7 +1075,7 @@
>  		  result))))
>  
>       ;; ??? Can delete if all cases are present.
> -     indent "  default : "
> +     indent "  default: "
>       (/gen-decode-default-entry invalid-insn fn?) "\n"
>       indent "  }\n"
>       indent "}\n"

Re: [PATCH] sim: tighten up generated decode tables

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

On 22 Dec 2023 11:55, Jose E. Marchesi wrote:
> > The use of /* fall through */ with consective case statements doesn't
> > really add any value, and when generating large files, can take up a
> > lot of space.  In the case of cris, it alone adds ~20k, or ~10%.
> 
> I am a little concern this change may trigger implicit-fallthrough
> warnings when compiling the generated code.  Not sure this is a problem
> in practice though, since nor binutils nor sim uses
> -Wimplicit-fallthrough for building as far as I can see.

pretty sure compilers don't warn about consecutive case statements that
don't have any non-case code inbetween.  so we're talking about:
          case 11:
          case 12:
          case 13:
          case 14: itype = CRISV10F_INSN_ADDOQ; goto extract_sfmt_addoq;

since this is a very common scenario, compilers accept it without warning.
if you did something like:
          case 13:
            printf("");
          case 14: itype = CRISV10F_INSN_ADDOQ; goto extract_sfmt_addoq;
then it'd warn.

i'm actually enabling -Wimplicit-fallthrough in sim now and none of these
trigger warnings.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] sim: tighten up generated decode tables

[Jose E. Marchesi]

> On 22 Dec 2023 11:55, Jose E. Marchesi wrote:
>> > The use of /* fall through */ with consective case statements doesn't
>> > really add any value, and when generating large files, can take up a
>> > lot of space.  In the case of cris, it alone adds ~20k, or ~10%.
>> 
>> I am a little concern this change may trigger implicit-fallthrough
>> warnings when compiling the generated code.  Not sure this is a problem
>> in practice though, since nor binutils nor sim uses
>> -Wimplicit-fallthrough for building as far as I can see.
>
> pretty sure compilers don't warn about consecutive case statements that
> don't have any non-case code inbetween.  so we're talking about:
>           case 11:
>           case 12:
>           case 13:
>           case 14: itype = CRISV10F_INSN_ADDOQ; goto extract_sfmt_addoq;
>
> since this is a very common scenario, compilers accept it without warning.
> if you did something like:
>           case 13:
>             printf("");
>           case 14: itype = CRISV10F_INSN_ADDOQ; goto extract_sfmt_addoq;
> then it'd warn.
>
> i'm actually enabling -Wimplicit-fallthrough in sim now and none of these
> trigger warnings.

OK then.
Thanks!

Re: [PATCH] sim: tighten up generated decode tables

[Frank Ch. Eigler]

Hi -

> The use of /* fall through */ with consective case statements doesn't
> really add any value, and when generating large files, can take up a
> lot of space.  In the case of cris, it alone adds ~20k, or ~10%.

A few kilobytes is basically zero cost, isn't it?

- FChE

Re: [PATCH] sim: tighten up generated decode tables

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

On 22 Dec 2023 11:12, Frank Ch. Eigler wrote:
> > The use of /* fall through */ with consective case statements doesn't
> > really add any value, and when generating large files, can take up a
> > lot of space.  In the case of cris, it alone adds ~20k, or ~10%.
> 
> A few kilobytes is basically zero cost, isn't it?

you're not necessarily wrong, although i find it easier to read without so
much noise.  i get that it's generated output, but when trying to debug and
understand the steps, having them be a bit readable is helpful.

i'll note that it's ~20k per file.  the sim tree has ~10 of these.  having
these seems like it adds up when using git as everyone has to carry the cost.

it's possible to compress the code even further if i was able to figure out
how the lisp works.  we generate hundreds of case lines that could be shrunk
into 1.  things like:
          case 0:
          case 1:
          case 2:
          case 3:
          case 4:
          case 5:
          case 6:
          case 7:
          case 8:
          case 9:
          case 10:
          case 11:
          case 12:
          case 13:
          case 14: itype = CRISV10F_INSN_ADDOQ; goto extract_sfmt_addoq;
could be:
          case 0 ... 14: itype = CRISV10F_INSN_ADDOQ; goto extract_sfmt_addoq;

some switches have 0 ... 127.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] sim: tighten up generated decode tables

[Frank Ch. Eigler]

Hi -

> > A few kilobytes is basically zero cost, isn't it?

OK, if its value is zero (because it does not impact compiler
current/forseeable warnings), then sure let's do away with it.

- FChE