* [PATCH] sim: avoid shadowing vars when decoding insns
@ 2023-12-22 0:55 Mike Frysinger
2023-12-22 10:45 ` Jose E. Marchesi
0 siblings, 1 reply; 3+ messages in thread
From: Mike Frysinger @ 2023-12-22 0:55 UTC (permalink / raw)
To: cgen
When generating switch decode tables, the switch statements all use
the same variable name "val". This causes nested switches to shadow
earlier ones. Use the existing "switch-num" to generate unique names
when nesting since it's otherwise unused.
Previously we'd have:
...
{
unsigned int val = (((insn >> 4) & (255 << 0)));
switch (val)
{
...
case 15 :
{
unsigned int val = (((insn >> 12) & (15 << 0)));
switch (val)
{
...
Leading to:
.../sim/cris/decodev10.c: In function ‘crisv10f_decode’:
.../sim/cris/decodev10.c:351:24: error: declaration of ‘val’ shadows a previous local [-Werror=shadow=compatible-local]
351 | unsigned int val = (((insn >> 12) & (15 << 0)));
| ^~~
.../sim/cris/decodev10.c:331:20: note: shadowed declaration is here
331 | unsigned int val = (((insn >> 4) & (255 << 0)));
| ^~~
Now we have:
...
{
unsigned int val0 = (((insn >> 4) & (255 << 0)));
switch (val0)
{
...
case 15 :
{
unsigned int val1 = (((insn >> 12) & (15 << 0)));
switch (val1)
{
...
---
utils-sim.scm | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/utils-sim.scm b/utils-sim.scm
index 07d776d286d8..dca0f0705036 100644
--- a/utils-sim.scm
+++ b/utils-sim.scm
@@ -995,8 +995,8 @@
)
; Generate switch statement to decode TABLE-GUTS.
-; SWITCH-NUM is for compatibility with the computed goto decoder and
-; isn't used.
+; SWITCH-NUM is for compatibility with the computed goto decoder and tracks the
+; nesting depth.
; STARTBIT is the bit offset of the instruction value that C variable `insn'
; holds (note that this is independent of LSB0?).
; DECODE-BITSIZE is the number of bits of the insn that `insn' holds.
@@ -1017,7 +1017,8 @@
table-guts table-guts-thus-far
indent lsb0? invalid-insn fn?)
- (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts))))
+ (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts)))
+ (varname (string-append "val" (number->string switch-num))))
(string-list
indent "{\n"
@@ -1027,19 +1028,19 @@
(set! startbit (dtable-guts-startbit table-guts))
(set! decode-bitsize (dtable-guts-bitsize table-guts))
;; FIXME: Bits may get fetched again during extraction.
- (string-append indent " unsigned int val;\n"
+ (string-append indent " unsigned int " varname ";\n"
indent " /* Must fetch more bits. */\n"
indent " insn = "
(gen-ifetch "pc" startbit decode-bitsize)
";\n"
- indent " val = "))
- (string-append indent " unsigned int val = "))
+ indent " " varname " = "))
+ (string-append indent " unsigned int " varname " = "))
(/gen-decode-bits (dtable-guts-bitnums table-guts)
(dtable-guts-startbit table-guts)
(dtable-guts-bitsize table-guts)
"insn" "entire_insn" lsb0?)
";\n"
- indent " switch (val)\n"
+ indent " switch (" varname ")\n"
indent " {\n"
;; The code is more readable, and icache use is improved, if we collapse
@@ -1067,7 +1068,7 @@
(/gen-decode-expr-entry (car entries) indent invalid-insn fn?))
((table)
(/gen-decode-table-entry (car entries) (cdr entries)
- switch-num startbit decode-bitsize
+ (+ switch-num 1) startbit decode-bitsize
new-table-guts-thus-far
indent lsb0? invalid-insn fn?))
)
@@ -1105,7 +1106,7 @@
; Now print it out.
- (/gen-decoder-switch "0" 0 decode-bitsize
+ (/gen-decoder-switch 0 0 decode-bitsize
table-guts nil
indent lsb0? invalid-insn fn?))
)
--
2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] sim: avoid shadowing vars when decoding insns
2023-12-22 0:55 [PATCH] sim: avoid shadowing vars when decoding insns Mike Frysinger
@ 2023-12-22 10:45 ` Jose E. Marchesi
2023-12-22 15:49 ` Mike Frysinger
0 siblings, 1 reply; 3+ messages in thread
From: Jose E. Marchesi @ 2023-12-22 10:45 UTC (permalink / raw)
To: Mike Frysinger via Cgen; +Cc: Mike Frysinger
Hi Mike.
> ; Generate switch statement to decode TABLE-GUTS.
> -; SWITCH-NUM is for compatibility with the computed goto decoder and
> -; isn't used.
> +; SWITCH-NUM is for compatibility with the computed goto decoder and tracks the
> +; nesting depth.
I wonder if it would be better to not mention the "computed goto
decoder" at all in the comment block, since AFAIK it doesn't
exist.
Other than that the patch is OK.
Thanks.
> ; STARTBIT is the bit offset of the instruction value that C variable `insn'
> ; holds (note that this is independent of LSB0?).
> ; DECODE-BITSIZE is the number of bits of the insn that `insn' holds.
> @@ -1017,7 +1017,8 @@
> table-guts table-guts-thus-far
> indent lsb0? invalid-insn fn?)
>
> - (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts))))
> + (let ((new-table-guts-thus-far (append table-guts-thus-far (list table-guts)))
> + (varname (string-append "val" (number->string switch-num))))
>
> (string-list
> indent "{\n"
> @@ -1027,19 +1028,19 @@
> (set! startbit (dtable-guts-startbit table-guts))
> (set! decode-bitsize (dtable-guts-bitsize table-guts))
> ;; FIXME: Bits may get fetched again during extraction.
> - (string-append indent " unsigned int val;\n"
> + (string-append indent " unsigned int " varname ";\n"
> indent " /* Must fetch more bits. */\n"
> indent " insn = "
> (gen-ifetch "pc" startbit decode-bitsize)
> ";\n"
> - indent " val = "))
> - (string-append indent " unsigned int val = "))
> + indent " " varname " = "))
> + (string-append indent " unsigned int " varname " = "))
> (/gen-decode-bits (dtable-guts-bitnums table-guts)
> (dtable-guts-startbit table-guts)
> (dtable-guts-bitsize table-guts)
> "insn" "entire_insn" lsb0?)
> ";\n"
> - indent " switch (val)\n"
> + indent " switch (" varname ")\n"
> indent " {\n"
>
> ;; The code is more readable, and icache use is improved, if we collapse
> @@ -1067,7 +1068,7 @@
> (/gen-decode-expr-entry (car entries) indent invalid-insn fn?))
> ((table)
> (/gen-decode-table-entry (car entries) (cdr entries)
> - switch-num startbit decode-bitsize
> + (+ switch-num 1) startbit decode-bitsize
> new-table-guts-thus-far
> indent lsb0? invalid-insn fn?))
> )
> @@ -1105,7 +1106,7 @@
>
> ; Now print it out.
>
> - (/gen-decoder-switch "0" 0 decode-bitsize
> + (/gen-decoder-switch 0 0 decode-bitsize
> table-guts nil
> indent lsb0? invalid-insn fn?))
> )
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-22 15:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-22 0:55 [PATCH] sim: avoid shadowing vars when decoding insns Mike Frysinger
2023-12-22 10:45 ` Jose E. Marchesi
2023-12-22 15:49 ` Mike Frysinger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).