disas/riscv.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
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.1On 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
>
>
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
>>
>>
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
> > >
> > >
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
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
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
>
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
>>
>
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
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 >
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 > >
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 >> >
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~
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~ >
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~
© 2016 - 2024 Red Hat, Inc.