[PATCH] disas/riscv: Further correction to LUI disassembly

[PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Bagley]

The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
by recovering the immediate argument from the result of LUI with a
shift right by 12. However, the shift right will left-fill with the
sign. By applying a mask we recover an unsigned representation of the
20-bit field (which includes a sign bit).

Example:
0xfffff000 >> 12 = 0xffffffff
0xfffff000 >> 12 & 0xfffff = 0x000fffff

Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper immediates")
Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
---
 disas/riscv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/disas/riscv.c b/disas/riscv.c
index 4023e3fc65..690eb4a1ac 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
             break;
         case 'U':
             fmt++;
-            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
-            append(buf, tmp, buflen);
-            if (*fmt == 'o') {
+            if (*fmt == 'i') {
+                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);
+                append(buf, tmp, buflen);
+            } else if (*fmt == 'o') {
+                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
+                append(buf, tmp, buflen);
                 while (strlen(buf) < tab * 2) {
                     append(buf, " ", buflen);
                 }
-- 
2.34.1

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Henderson]

On 7/31/23 11:33, Richard Bagley wrote:
> The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> by recovering the immediate argument from the result of LUI with a
> shift right by 12. However, the shift right will left-fill with the
> sign. By applying a mask we recover an unsigned representation of the
> 20-bit field (which includes a sign bit).

Why would you want that?  Surely

     lui r1, -1

is more accurate than

     lui r1, 0xfffff


r~

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Bagley]

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

I do apologize, but I do not understand your remark at all. Could I trouble
you to spell this out.

In:
+                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);
0xfffff is a mask which recovers the 20 bit field used to represent the
immediate in the instruction encoding.

You seem to be responding to the syntax, which is unrelated to my change.
But I did notice it is the case that both GCC and LLVM disassemblers do not
accept signed integer arguments to LUI:
lui r1, -1
but instead require
lui r1, 0xfffff
I don't see why the former is more accurate, but it would be an aid to the
assembly programmer.

I have recommended internally that if the current format cannot support
both, then it might be worthwhile to propose a pseudo instruction for RISCV
for precisely this syntax variant:
lui.s r1.-1

Richard

On Mon, Jul 31, 2023 at 1:37 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 7/31/23 11:33, Richard Bagley wrote:
> > The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> > by recovering the immediate argument from the result of LUI with a
> > shift right by 12. However, the shift right will left-fill with the
> > sign. By applying a mask we recover an unsigned representation of the
> > 20-bit field (which includes a sign bit).
>
> Why would you want that?  Surely
>
>      lui r1, -1
>
> is more accurate than
>
>      lui r1, 0xfffff
>
>
> r~
>

[-- Attachment #2: Type: text/html, Size: 1870 bytes --]

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Henderson]

On 8/7/23 15:01, Richard Bagley wrote:
> I do apologize, but I do not understand your remark at all. Could I trouble you to spell 
> this out.
> 
> In:
> +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);
> 0xfffff is a mask which recovers the 20 bit field used to represent the immediate in the 
> instruction encoding.
> 
> You seem to be responding to the syntax, which is unrelated to my change.
> But I did notice it is the case that both GCC and LLVM disassemblers do not accept signed 
> integer arguments to LUI:
> lui r1, -1
> but instead require
> lui r1, 0xfffff

Your language is confusing.  Disassembler or assembler?  A disassembler would not "accept" 
but "output" arguments for LUI.

If the assembler rejects "lui r1, -1", that would be a bug, because the field *is* signed.

For the disassembler, the field *is* signed, therefore outputting a signed value is 
correct.  Outputting an unsigned hex value hides the fact that bit 31 is the sign.

To my mind this is exactly the same as emitting a signed value for the immediate in ADDI.


r~

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> by recovering the immediate argument from the result of LUI with a
> shift right by 12. However, the shift right will left-fill with the
> sign. By applying a mask we recover an unsigned representation of the
> 20-bit field (which includes a sign bit).
> 
> Example:
> 0xfffff000 >> 12 = 0xffffffff
> 0xfffff000 >> 12 & 0xfffff = 0x000fffff
> 
> Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper immediates")
> Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
> ---
>  disas/riscv.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/disas/riscv.c b/disas/riscv.c
> index 4023e3fc65..690eb4a1ac 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
>              break;
>          case 'U':
>              fmt++;
> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> -            append(buf, tmp, buflen);
> -            if (*fmt == 'o') {
> +            if (*fmt == 'i') {
> +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);

Why are we correcting LUI's output, but still outputting sign-extended
values for AUIPC?

We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1' without getting

 Error: lui expression not in range 0..1048575

(and additionally for 0xffffffff)

 Error: value of 00000ffffffff000 too large for field of 4 bytes at 0000000000000000

either.

(I see that the assembler's error messages state 'lui', but I was trying
'auipc'.)

I'm using as from gnu binutils 2.40.0.20230214.

(And, FWIW, I agree with Richard Henderson that these instructions should
accept negative values.)

Thanks,
drew


> +                append(buf, tmp, buflen);
> +            } else if (*fmt == 'o') {
> +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> +                append(buf, tmp, buflen);
>                  while (strlen(buf) < tab * 2) {
>                      append(buf, " ", buflen);
>                  }
> -- 
> 2.34.1
> 
> 

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Palmer Dabbelt]

On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajones@ventanamicro.com wrote:
> On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
>> The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
>> by recovering the immediate argument from the result of LUI with a
>> shift right by 12. However, the shift right will left-fill with the
>> sign. By applying a mask we recover an unsigned representation of the
>> 20-bit field (which includes a sign bit).
>>
>> Example:
>> 0xfffff000 >> 12 = 0xffffffff
>> 0xfffff000 >> 12 & 0xfffff = 0x000fffff
>>
>> Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper immediates")
>> Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
>> ---
>>  disas/riscv.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/disas/riscv.c b/disas/riscv.c
>> index 4023e3fc65..690eb4a1ac 100644
>> --- a/disas/riscv.c
>> +++ b/disas/riscv.c
>> @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
>>              break;
>>          case 'U':
>>              fmt++;
>> -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
>> -            append(buf, tmp, buflen);
>> -            if (*fmt == 'o') {
>> +            if (*fmt == 'i') {
>> +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);
>
> Why are we correcting LUI's output, but still outputting sign-extended
> values for AUIPC?
>
> We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1' without getting
>
>  Error: lui expression not in range 0..1048575
>
> (and additionally for 0xffffffff)
>
>  Error: value of 00000ffffffff000 too large for field of 4 bytes at 0000000000000000
>
> either.
>
> (I see that the assembler's error messages state 'lui', but I was trying
> 'auipc'.)
>
> I'm using as from gnu binutils 2.40.0.20230214.
>
> (And, FWIW, I agree with Richard Henderson that these instructions should
> accept negative values.)

I'm kind of lost here, and you saying binutils rejects this syntax?  If 
that's the case it's probably just an oversight, can you file a bug in 
binutils land so folks can see?

>
> Thanks,
> drew
>
>
>> +                append(buf, tmp, buflen);
>> +            } else if (*fmt == 'o') {
>> +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
>> +                append(buf, tmp, buflen);
>>                  while (strlen(buf) < tab * 2) {
>>                      append(buf, " ", buflen);
>>                  }
>> --
>> 2.34.1
>>
>>

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
> On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajones@ventanamicro.com wrote:
> > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> > > The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> > > by recovering the immediate argument from the result of LUI with a
> > > shift right by 12. However, the shift right will left-fill with the
> > > sign. By applying a mask we recover an unsigned representation of the
> > > 20-bit field (which includes a sign bit).
> > > 
> > > Example:
> > > 0xfffff000 >> 12 = 0xffffffff
> > > 0xfffff000 >> 12 & 0xfffff = 0x000fffff
> > > 
> > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper immediates")
> > > Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
> > > ---
> > >  disas/riscv.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > index 4023e3fc65..690eb4a1ac 100644
> > > --- a/disas/riscv.c
> > > +++ b/disas/riscv.c
> > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
> > >              break;
> > >          case 'U':
> > >              fmt++;
> > > -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > -            append(buf, tmp, buflen);
> > > -            if (*fmt == 'o') {
> > > +            if (*fmt == 'i') {
> > > +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);
> > 
> > Why are we correcting LUI's output, but still outputting sign-extended
> > values for AUIPC?
> > 
> > We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1' without getting
> > 
> >  Error: lui expression not in range 0..1048575
> > 
> > (and additionally for 0xffffffff)
> > 
> >  Error: value of 00000ffffffff000 too large for field of 4 bytes at 0000000000000000
> > 
> > either.
> > 
> > (I see that the assembler's error messages state 'lui', but I was trying
> > 'auipc'.)
> > 
> > I'm using as from gnu binutils 2.40.0.20230214.
> > 
> > (And, FWIW, I agree with Richard Henderson that these instructions should
> > accept negative values.)
> 
> I'm kind of lost here, and you saying binutils rejects this syntax?  If
> that's the case it's probably just an oversight, can you file a bug in
> binutils land so folks can see?

Will do.

Thanks,
drew

> 
> > 
> > Thanks,
> > drew
> > 
> > 
> > > +                append(buf, tmp, buflen);
> > > +            } else if (*fmt == 'o') {
> > > +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > +                append(buf, tmp, buflen);
> > >                  while (strlen(buf) < tab * 2) {
> > >                      append(buf, " ", buflen);
> > >                  }
> > > --
> > > 2.34.1
> > > 
> > > 

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Thu, Aug 10, 2023 at 06:27:50PM +0200, Andrew Jones wrote:
> On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
> > On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajones@ventanamicro.com wrote:
> > > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> > > > The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> > > > by recovering the immediate argument from the result of LUI with a
> > > > shift right by 12. However, the shift right will left-fill with the
> > > > sign. By applying a mask we recover an unsigned representation of the
> > > > 20-bit field (which includes a sign bit).
> > > > 
> > > > Example:
> > > > 0xfffff000 >> 12 = 0xffffffff
> > > > 0xfffff000 >> 12 & 0xfffff = 0x000fffff
> > > > 
> > > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper immediates")
> > > > Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
> > > > ---
> > > >  disas/riscv.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > > index 4023e3fc65..690eb4a1ac 100644
> > > > --- a/disas/riscv.c
> > > > +++ b/disas/riscv.c
> > > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
> > > >              break;
> > > >          case 'U':
> > > >              fmt++;
> > > > -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > > -            append(buf, tmp, buflen);
> > > > -            if (*fmt == 'o') {
> > > > +            if (*fmt == 'i') {
> > > > +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);
> > > 
> > > Why are we correcting LUI's output, but still outputting sign-extended
> > > values for AUIPC?
> > > 
> > > We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1' without getting
> > > 
> > >  Error: lui expression not in range 0..1048575
> > > 
> > > (and additionally for 0xffffffff)
> > > 
> > >  Error: value of 00000ffffffff000 too large for field of 4 bytes at 0000000000000000
> > > 
> > > either.
> > > 
> > > (I see that the assembler's error messages state 'lui', but I was trying
> > > 'auipc'.)
> > > 
> > > I'm using as from gnu binutils 2.40.0.20230214.
> > > 
> > > (And, FWIW, I agree with Richard Henderson that these instructions should
> > > accept negative values.)
> > 
> > I'm kind of lost here, and you saying binutils rejects this syntax?  If
> > that's the case it's probably just an oversight, can you file a bug in
> > binutils land so folks can see?
> 
> Will do.
>

https://sourceware.org/bugzilla/show_bug.cgi?id=30746

Thanks,
drew

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Fri, Aug 11, 2023 at 10:25:52AM +0200, Andrew Jones wrote:
> On Thu, Aug 10, 2023 at 06:27:50PM +0200, Andrew Jones wrote:
> > On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
> > > On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajones@ventanamicro.com wrote:
> > > > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> > > > > The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> > > > > by recovering the immediate argument from the result of LUI with a
> > > > > shift right by 12. However, the shift right will left-fill with the
> > > > > sign. By applying a mask we recover an unsigned representation of the
> > > > > 20-bit field (which includes a sign bit).
> > > > > 
> > > > > Example:
> > > > > 0xfffff000 >> 12 = 0xffffffff
> > > > > 0xfffff000 >> 12 & 0xfffff = 0x000fffff
> > > > > 
> > > > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper immediates")
> > > > > Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
> > > > > ---
> > > > >  disas/riscv.c | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > > > index 4023e3fc65..690eb4a1ac 100644
> > > > > --- a/disas/riscv.c
> > > > > +++ b/disas/riscv.c
> > > > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t buflen, size_t tab, rv_decode *dec)
> > > > >              break;
> > > > >          case 'U':
> > > > >              fmt++;
> > > > > -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > > > -            append(buf, tmp, buflen);
> > > > > -            if (*fmt == 'o') {
> > > > > +            if (*fmt == 'i') {
> > > > > +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 0xfffff);
> > > > 
> > > > Why are we correcting LUI's output, but still outputting sign-extended
> > > > values for AUIPC?
> > > > 
> > > > We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1' without getting
> > > > 
> > > >  Error: lui expression not in range 0..1048575
> > > > 
> > > > (and additionally for 0xffffffff)
> > > > 
> > > >  Error: value of 00000ffffffff000 too large for field of 4 bytes at 0000000000000000
> > > > 
> > > > either.
> > > > 
> > > > (I see that the assembler's error messages state 'lui', but I was trying
> > > > 'auipc'.)
> > > > 
> > > > I'm using as from gnu binutils 2.40.0.20230214.
> > > > 
> > > > (And, FWIW, I agree with Richard Henderson that these instructions should
> > > > accept negative values.)
> > > 
> > > I'm kind of lost here, and you saying binutils rejects this syntax?  If
> > > that's the case it's probably just an oversight, can you file a bug in
> > > binutils land so folks can see?
> > 
> > Will do.
> >
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=30746
>

But, to try to bring this thread back to the patch under review. While the
binutils BZ may address our preferred way of providing immediates to the
assembler, this patch is trying to make QEMU's output consistent with
objdump. Since objdump always outputs long immediate values as hex, then
it doesn't need to care about negative signs. QEMU seems to prefer
decimal, though, and so does llvm-objdump, which outputs values for these
instructions in the range 0..1048575. So, I guess this patch is making
QEMU consistent with llvm-objdump.

Back to making suggestions for this patch...

1. The commit message should probably say something along the lines of
   what I just wrote in the preceding paragraph to better explain the
   motivation.

2. Unless I'm missing something, then this patch should also address
   AUIPC.

Thanks,
drew

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Bagley]

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

NACK

We have established that the change is a workaround for a bug in
the assembler.
I withdraw the merge request.

Thank you for this careful review.

On Fri, Aug 11, 2023 at 4:55 AM Andrew Jones <ajones@ventanamicro.com>
wrote:

> On Fri, Aug 11, 2023 at 10:25:52AM +0200, Andrew Jones wrote:
> > On Thu, Aug 10, 2023 at 06:27:50PM +0200, Andrew Jones wrote:
> > > On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
> > > > On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajones@ventanamicro.com
> wrote:
> > > > > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> > > > > > The recent commit 36df75a0a9 corrected one aspect of LUI
> disassembly
> > > > > > by recovering the immediate argument from the result of LUI with
> a
> > > > > > shift right by 12. However, the shift right will left-fill with
> the
> > > > > > sign. By applying a mask we recover an unsigned representation
> of the
> > > > > > 20-bit field (which includes a sign bit).
> > > > > >
> > > > > > Example:
> > > > > > 0xfffff000 >> 12 = 0xffffffff
> > > > > > 0xfffff000 >> 12 & 0xfffff = 0x000fffff
> > > > > >
> > > > > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper
> immediates")
> > > > > > Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
> > > > > > ---
> > > > > >  disas/riscv.c | 9 ++++++---
> > > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > > > > index 4023e3fc65..690eb4a1ac 100644
> > > > > > --- a/disas/riscv.c
> > > > > > +++ b/disas/riscv.c
> > > > > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t
> buflen, size_t tab, rv_decode *dec)
> > > > > >              break;
> > > > > >          case 'U':
> > > > > >              fmt++;
> > > > > > -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > > > > -            append(buf, tmp, buflen);
> > > > > > -            if (*fmt == 'o') {
> > > > > > +            if (*fmt == 'i') {
> > > > > > +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12
> & 0xfffff);
> > > > >
> > > > > Why are we correcting LUI's output, but still outputting
> sign-extended
> > > > > values for AUIPC?
> > > > >
> > > > > We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1' without
> getting
> > > > >
> > > > >  Error: lui expression not in range 0..1048575
> > > > >
> > > > > (and additionally for 0xffffffff)
> > > > >
> > > > >  Error: value of 00000ffffffff000 too large for field of 4 bytes
> at 0000000000000000
> > > > >
> > > > > either.
> > > > >
> > > > > (I see that the assembler's error messages state 'lui', but I was
> trying
> > > > > 'auipc'.)
> > > > >
> > > > > I'm using as from gnu binutils 2.40.0.20230214.
> > > > >
> > > > > (And, FWIW, I agree with Richard Henderson that these instructions
> should
> > > > > accept negative values.)
> > > >
> > > > I'm kind of lost here, and you saying binutils rejects this syntax?
> If
> > > > that's the case it's probably just an oversight, can you file a bug
> in
> > > > binutils land so folks can see?
> > >
> > > Will do.
> > >
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=30746
> >
>
> But, to try to bring this thread back to the patch under review. While the
> binutils BZ may address our preferred way of providing immediates to the
> assembler, this patch is trying to make QEMU's output consistent with
> objdump. Since objdump always outputs long immediate values as hex, then
> it doesn't need to care about negative signs. QEMU seems to prefer
> decimal, though, and so does llvm-objdump, which outputs values for these
> instructions in the range 0..1048575. So, I guess this patch is making
> QEMU consistent with llvm-objdump.
>
> Back to making suggestions for this patch...
>
> 1. The commit message should probably say something along the lines of
>    what I just wrote in the preceding paragraph to better explain the
>    motivation.
>
> 2. Unless I'm missing something, then this patch should also address
>    AUIPC.
>
> Thanks,
> drew
>

[-- Attachment #2: Type: text/html, Size: 5832 bytes --]

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Bagley]

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

post-nack, one further comment:

One could argue that this change also aligns QEMU with supporting tools (as
Andrew observed), and it makes sense to merge this change into QEMU until
those tools update to supporting signed decimal numbers with immediates.

As it is, both GNU assembler and the LLVM integrated assembler (or llvm-mc)
throws an error with examples such as
auipc s0, -17

On the other hand, I have only seen this problem with the output of the
COLLECT plug-in, not (as yet) with QEMU execution proper.
If the problem is confined to COLLECT, perhaps the argument for aligning
with other tools is not as strong.

In the meantime, I have adjusted my change locally to include AUIPC, and
written a substantive, and I hope, clear commit description.
If you would like me to resubmit a patch with this updated change, please
let me know.



On Thu, Mar 7, 2024 at 4:08 PM Richard Bagley <rbagley@ventanamicro.com>
wrote:

> NACK
>
> We have established that the change is a workaround for a bug in
> the assembler.
> I withdraw the merge request.
>
> Thank you for this careful review.
>
> On Fri, Aug 11, 2023 at 4:55 AM Andrew Jones <ajones@ventanamicro.com>
> wrote:
>
>> On Fri, Aug 11, 2023 at 10:25:52AM +0200, Andrew Jones wrote:
>> > On Thu, Aug 10, 2023 at 06:27:50PM +0200, Andrew Jones wrote:
>> > > On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
>> > > > On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajones@ventanamicro.com
>> wrote:
>> > > > > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
>> > > > > > The recent commit 36df75a0a9 corrected one aspect of LUI
>> disassembly
>> > > > > > by recovering the immediate argument from the result of LUI
>> with a
>> > > > > > shift right by 12. However, the shift right will left-fill with
>> the
>> > > > > > sign. By applying a mask we recover an unsigned representation
>> of the
>> > > > > > 20-bit field (which includes a sign bit).
>> > > > > >
>> > > > > > Example:
>> > > > > > 0xfffff000 >> 12 = 0xffffffff
>> > > > > > 0xfffff000 >> 12 & 0xfffff = 0x000fffff
>> > > > > >
>> > > > > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper
>> immediates")
>> > > > > > Signed-off-by: Richard Bagley <rbagley@ventanamicro.com>
>> > > > > > ---
>> > > > > >  disas/riscv.c | 9 ++++++---
>> > > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
>> > > > > >
>> > > > > > diff --git a/disas/riscv.c b/disas/riscv.c
>> > > > > > index 4023e3fc65..690eb4a1ac 100644
>> > > > > > --- a/disas/riscv.c
>> > > > > > +++ b/disas/riscv.c
>> > > > > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf,
>> size_t buflen, size_t tab, rv_decode *dec)
>> > > > > >              break;
>> > > > > >          case 'U':
>> > > > > >              fmt++;
>> > > > > > -            snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
>> > > > > > -            append(buf, tmp, buflen);
>> > > > > > -            if (*fmt == 'o') {
>> > > > > > +            if (*fmt == 'i') {
>> > > > > > +                snprintf(tmp, sizeof(tmp), "%d", dec->imm >>
>> 12 & 0xfffff);
>> > > > >
>> > > > > Why are we correcting LUI's output, but still outputting
>> sign-extended
>> > > > > values for AUIPC?
>> > > > >
>> > > > > We can't assemble 'auipc a1, 0xffffffff' or 'auipc a1, -1'
>> without getting
>> > > > >
>> > > > >  Error: lui expression not in range 0..1048575
>> > > > >
>> > > > > (and additionally for 0xffffffff)
>> > > > >
>> > > > >  Error: value of 00000ffffffff000 too large for field of 4 bytes
>> at 0000000000000000
>> > > > >
>> > > > > either.
>> > > > >
>> > > > > (I see that the assembler's error messages state 'lui', but I was
>> trying
>> > > > > 'auipc'.)
>> > > > >
>> > > > > I'm using as from gnu binutils 2.40.0.20230214.
>> > > > >
>> > > > > (And, FWIW, I agree with Richard Henderson that these
>> instructions should
>> > > > > accept negative values.)
>> > > >
>> > > > I'm kind of lost here, and you saying binutils rejects this
>> syntax?  If
>> > > > that's the case it's probably just an oversight, can you file a bug
>> in
>> > > > binutils land so folks can see?
>> > >
>> > > Will do.
>> > >
>> >
>> > https://sourceware.org/bugzilla/show_bug.cgi?id=30746
>> >
>>
>> But, to try to bring this thread back to the patch under review. While the
>> binutils BZ may address our preferred way of providing immediates to the
>> assembler, this patch is trying to make QEMU's output consistent with
>> objdump. Since objdump always outputs long immediate values as hex, then
>> it doesn't need to care about negative signs. QEMU seems to prefer
>> decimal, though, and so does llvm-objdump, which outputs values for these
>> instructions in the range 0..1048575. So, I guess this patch is making
>> QEMU consistent with llvm-objdump.
>>
>> Back to making suggestions for this patch...
>>
>> 1. The commit message should probably say something along the lines of
>>    what I just wrote in the preceding paragraph to better explain the
>>    motivation.
>>
>> 2. Unless I'm missing something, then this patch should also address
>>    AUIPC.
>>
>> Thanks,
>> drew
>>
>

[-- Attachment #2: Type: text/html, Size: 7110 bytes --]

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Fri, Mar 08, 2024 at 08:22:01PM -0800, Richard Bagley wrote:
> post-nack, one further comment:
> 
> One could argue that this change also aligns QEMU with supporting tools (as
> Andrew observed), and it makes sense to merge this change into QEMU until
> those tools update to supporting signed decimal numbers with immediates.
> 
> As it is, both GNU assembler and the LLVM integrated assembler (or llvm-mc)
> throws an error with examples such as
> auipc s0, -17
> 
> On the other hand, I have only seen this problem with the output of the
> COLLECT plug-in, not (as yet) with QEMU execution proper.
> If the problem is confined to COLLECT, perhaps the argument for aligning
> with other tools is not as strong.
> 
> In the meantime, I have adjusted my change locally to include AUIPC, and
> written a substantive, and I hope, clear commit description.
> If you would like me to resubmit a patch with this updated change, please
> let me know.

Since the patch is ready for posting, then it might as well be posted
(even if it may not get merged right away). If the issue arises again,
then we can refer to the latest proposed patch, which will be preserved
in the mail archives.

Thanks,
drew

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Bagley]

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

I have realized that *the patch is indeed a fix*, not a workaround.

In fact, the argument to LUI and AUIPC in assembly *must* be a number
between [0x0, 0xfffff].
RISC-V Assembly Programmer's Manual : Load Upper Immediate's Immediate
<https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md#load-upper-immediates-immediate>
Signed decimal numbers are programmed as their two's complement.

I checked: neither GCC nor LLVM will assemble

> lui x1, -4

The LLVM compiled models the arguments to LUI and AUIPC as UIMM (unsigned
immediate) or UIMM20 (20 bit unsigned immediate).

I should have checked this from the start. I jumped to the conclusion that
both formats (signed decimal, two's complement) for negative arguments
should be supported, and that I was encountering a bug.
I apologize to all for the unnecessary back-and-forth.

I don't yet see a reason why llvm and gcc could not support a signed number
in decimal format, perhaps requiring a pseudo-instruction.
This might be desirable, if only in support of assembly programming.
On the other hand, it is easy to make the conversion to a two's-complement
number.

Richard

On Sat, Mar 9, 2024 at 4:01 AM Andrew Jones <ajones@ventanamicro.com> wrote:

> On Fri, Mar 08, 2024 at 08:22:01PM -0800, Richard Bagley wrote:
> > post-nack, one further comment:
> >
> > One could argue that this change also aligns QEMU with supporting tools
> (as
> > Andrew observed), and it makes sense to merge this change into QEMU until
> > those tools update to supporting signed decimal numbers with immediates.
> >
> > As it is, both GNU assembler and the LLVM integrated assembler (or
> llvm-mc)
> > throws an error with examples such as
> > auipc s0, -17
> >
> > On the other hand, I have only seen this problem with the output of the
> > COLLECT plug-in, not (as yet) with QEMU execution proper.
> > If the problem is confined to COLLECT, perhaps the argument for aligning
> > with other tools is not as strong.
> >
> > In the meantime, I have adjusted my change locally to include AUIPC, and
> > written a substantive, and I hope, clear commit description.
> > If you would like me to resubmit a patch with this updated change, please
> > let me know.
>
> Since the patch is ready for posting, then it might as well be posted
> (even if it may not get merged right away). If the issue arises again,
> then we can refer to the latest proposed patch, which will be preserved
> in the mail archives.
>
> Thanks,
> drew
>

[-- Attachment #2: Type: text/html, Size: 3585 bytes --]

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Richard Bagley]

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

LUI and AUIPC would naturally be used to build an address.
If you want to supply a signed decimal number to build an immediate, there
is always the pseudo-instruction LI.

On Mon, Mar 11, 2024 at 11:56 AM Richard Bagley <rbagley@ventanamicro.com>
wrote:

> I have realized that *the patch is indeed a fix*, not a workaround.
>
> In fact, the argument to LUI and AUIPC in assembly *must* be a number
> between [0x0, 0xfffff].
> RISC-V Assembly Programmer's Manual : Load Upper Immediate's Immediate
> <https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md#load-upper-immediates-immediate>
> Signed decimal numbers are programmed as their two's complement.
>
> I checked: neither GCC nor LLVM will assemble
>
>> lui x1, -4
>
> The LLVM compiled models the arguments to LUI and AUIPC as UIMM (unsigned
> immediate) or UIMM20 (20 bit unsigned immediate).
>
> I should have checked this from the start. I jumped to the conclusion that
> both formats (signed decimal, two's complement) for negative arguments
> should be supported, and that I was encountering a bug.
> I apologize to all for the unnecessary back-and-forth.
>
> I don't yet see a reason why llvm and gcc could not support a signed
> number in decimal format, perhaps requiring a pseudo-instruction.
> This might be desirable, if only in support of assembly programming.
> On the other hand, it is easy to make the conversion to a two's-complement
> number.
>
> Richard
>
> On Sat, Mar 9, 2024 at 4:01 AM Andrew Jones <ajones@ventanamicro.com>
> wrote:
>
>> On Fri, Mar 08, 2024 at 08:22:01PM -0800, Richard Bagley wrote:
>> > post-nack, one further comment:
>> >
>> > One could argue that this change also aligns QEMU with supporting tools
>> (as
>> > Andrew observed), and it makes sense to merge this change into QEMU
>> until
>> > those tools update to supporting signed decimal numbers with immediates.
>> >
>> > As it is, both GNU assembler and the LLVM integrated assembler (or
>> llvm-mc)
>> > throws an error with examples such as
>> > auipc s0, -17
>> >
>> > On the other hand, I have only seen this problem with the output of the
>> > COLLECT plug-in, not (as yet) with QEMU execution proper.
>> > If the problem is confined to COLLECT, perhaps the argument for aligning
>> > with other tools is not as strong.
>> >
>> > In the meantime, I have adjusted my change locally to include AUIPC, and
>> > written a substantive, and I hope, clear commit description.
>> > If you would like me to resubmit a patch with this updated change,
>> please
>> > let me know.
>>
>> Since the patch is ready for posting, then it might as well be posted
>> (even if it may not get merged right away). If the issue arises again,
>> then we can refer to the latest proposed patch, which will be preserved
>> in the mail archives.
>>
>> Thanks,
>> drew
>>
>

[-- Attachment #2: Type: text/html, Size: 4161 bytes --]

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Mon, Mar 11, 2024 at 11:56:42AM -0700, Richard Bagley wrote:
> I have realized that *the patch is indeed a fix*, not a workaround.
> 
> In fact, the argument to LUI and AUIPC in assembly *must* be a number
> between [0x0, 0xfffff].
> RISC-V Assembly Programmer's Manual : Load Upper Immediate's Immediate
> <https://github.com/riscv-non-isa/riscv-asm-manual/blob/master/riscv-asm.md#load-upper-immediates-immediate>

I think that's just documenting the current behavior, but the behavior
(not accepting a signed decimal number for a signed immediate) doesn't
appear to be justified, so I think my suggestion in [1] still stands.
That said, I don't really have much of a horse in this race so if
somebody comes along and closes that BZ with a simple justification of
"we, the people that work on this stuff, agreed we prefer the range
[0x0, 0xfffff]", then I won't argue.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=30746

Thanks,
drew

> Signed decimal numbers are programmed as their two's complement.
> 
> I checked: neither GCC nor LLVM will assemble
> 
> > lui x1, -4
> 
> The LLVM compiled models the arguments to LUI and AUIPC as UIMM (unsigned
> immediate) or UIMM20 (20 bit unsigned immediate).
> 
> I should have checked this from the start. I jumped to the conclusion that
> both formats (signed decimal, two's complement) for negative arguments
> should be supported, and that I was encountering a bug.
> I apologize to all for the unnecessary back-and-forth.
> 
> I don't yet see a reason why llvm and gcc could not support a signed number
> in decimal format, perhaps requiring a pseudo-instruction.
> This might be desirable, if only in support of assembly programming.
> On the other hand, it is easy to make the conversion to a two's-complement
> number.
> 
> Richard
> 
> On Sat, Mar 9, 2024 at 4:01 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> 
> > On Fri, Mar 08, 2024 at 08:22:01PM -0800, Richard Bagley wrote:
> > > post-nack, one further comment:
> > >
> > > One could argue that this change also aligns QEMU with supporting tools
> > (as
> > > Andrew observed), and it makes sense to merge this change into QEMU until
> > > those tools update to supporting signed decimal numbers with immediates.
> > >
> > > As it is, both GNU assembler and the LLVM integrated assembler (or
> > llvm-mc)
> > > throws an error with examples such as
> > > auipc s0, -17
> > >
> > > On the other hand, I have only seen this problem with the output of the
> > > COLLECT plug-in, not (as yet) with QEMU execution proper.
> > > If the problem is confined to COLLECT, perhaps the argument for aligning
> > > with other tools is not as strong.
> > >
> > > In the meantime, I have adjusted my change locally to include AUIPC, and
> > > written a substantive, and I hope, clear commit description.
> > > If you would like me to resubmit a patch with this updated change, please
> > > let me know.
> >
> > Since the patch is ready for posting, then it might as well be posted
> > (even if it may not get merged right away). If the issue arises again,
> > then we can refer to the latest proposed patch, which will be preserved
> > in the mail archives.
> >
> > Thanks,
> > drew
> >