[PATCH] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[PATCH] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Jin Ma]

When using  '%ld' to print 'long long int' variable, 'fprintf' will
produce messy output on a 32-bit system, in an incorrect instruction
being generated, such as 'th.lwib a1,(a0),-16,4294967295'. And the
following error occurred during compilation:

Assembler messages:
Error: improper immediate value (18446744073709551615)

gcc/ChangeLog:

	* config/riscv/thead.cc (th_print_operand_address): Change %ld
	to %lld.
---
 gcc/config/riscv/thead.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
index 2955bc5f8a9..9ee6444b627 100644
--- a/gcc/config/riscv/thead.cc
+++ b/gcc/config/riscv/thead.cc
@@ -1141,7 +1141,7 @@ th_print_operand_address (FILE *file, machine_mode mode, rtx x)
       return true;
 
     case ADDRESS_REG_WB:
-      fprintf (file, "(%s),%ld,%u", reg_names[REGNO (addr.reg)],
+      fprintf (file, "(%s),%lld,%u", reg_names[REGNO (addr.reg)],
 	       INTVAL (addr.offset) >> addr.shift, addr.shift);
 	return true;
 
-- 
2.17.1

Re: [PATCH] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Andrew Pinski]

On Mon, Jan 29, 2024 at 1:21 AM Jin Ma <jinma@linux.alibaba.com> wrote:
>
> When using  '%ld' to print 'long long int' variable, 'fprintf' will
> produce messy output on a 32-bit system, in an incorrect instruction
> being generated, such as 'th.lwib a1,(a0),-16,4294967295'. And the
> following error occurred during compilation:
>
> Assembler messages:
> Error: improper immediate value (18446744073709551615)
>
> gcc/ChangeLog:
>
>         * config/riscv/thead.cc (th_print_operand_address): Change %ld
>         to %lld.
> ---
>  gcc/config/riscv/thead.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
> index 2955bc5f8a9..9ee6444b627 100644
> --- a/gcc/config/riscv/thead.cc
> +++ b/gcc/config/riscv/thead.cc
> @@ -1141,7 +1141,7 @@ th_print_operand_address (FILE *file, machine_mode mode, rtx x)
>        return true;
>
>      case ADDRESS_REG_WB:
> -      fprintf (file, "(%s),%ld,%u", reg_names[REGNO (addr.reg)],
> +      fprintf (file, "(%s),%lld,%u", reg_names[REGNO (addr.reg)],
>                INTVAL (addr.offset) >> addr.shift, addr.shift);


This is wrong, you should instead use HOST_WIDE_INT_PRINT_DEC or
HOST_WIDE_INT_PRINT_UNSIGNED.

Thanks,
Andrew Pinski

>         return true;
>
> --
> 2.17.1
>

Re:[PATCH] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Jin Ma]

>On Mon, Jan 29, 2024 at 1:21=E2=80=AFAM Jin Ma <jinma@linux.alibaba.com> wr=
>ote:
>>
>> When using  '%ld' to print 'long long int' variable, 'fprintf' will
>> produce messy output on a 32-bit system, in an incorrect instruction
>> being generated, such as 'th.lwib a1,(a0),-16,4294967295'. And the
>> following error occurred during compilation:
>>
>> Assembler messages:
>> Error: improper immediate value (18446744073709551615)
>>
>> gcc/ChangeLog:
>>
>>         * config/riscv/thead.cc (th_print_operand_address): Change %ld
>>         to %lld.
>> ---
>>  gcc/config/riscv/thead.cc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
>> index 2955bc5f8a9..9ee6444b627 100644
>> --- a/gcc/config/riscv/thead.cc
>> +++ b/gcc/config/riscv/thead.cc
>> @@ -1141,7 +1141,7 @@ th_print_operand_address (FILE *file, machine_mode =
>mode, rtx x)
>>        return true;
>>
>>      case ADDRESS_REG_WB:
>> -      fprintf (file, "(%s),%ld,%u", reg_names[REGNO (addr.reg)],
>> +      fprintf (file, "(%s),%lld,%u", reg_names[REGNO (addr.reg)],
>>                INTVAL (addr.offset) >> addr.shift, addr.shift);


>This is wrong, you should instead use HOST_WIDE_INT_PRINT_DEC or
>HOST_WIDE_INT_PRINT_UNSIGNED.

>Thanks,
>Andrew Pinski

Yes, thank you very much for your guidance. It will be better to
use HOST_WIDE_INT_PRINT_DEC. I will make changes later :)

BR

Jin

>>         return true;

[PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Jin Ma]

When using  '%ld' to print 'long long int' variable, 'fprintf' will
produce messy output on a 32-bit system, in an incorrect instruction
being generated, such as 'th.lwib a1,(a0),-16,4294967295'. And the
following error occurred during compilation:

Assembler messages:
Error: improper immediate value (18446744073709551615)

gcc/ChangeLog:

	* config/riscv/thead.cc (th_print_operand_address): Change %ld
	to %lld.
---
 gcc/config/riscv/thead.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
index 2955bc5f8a9..e4b8c37bc28 100644
--- a/gcc/config/riscv/thead.cc
+++ b/gcc/config/riscv/thead.cc
@@ -1141,7 +1141,7 @@ th_print_operand_address (FILE *file, machine_mode mode, rtx x)
       return true;
 
     case ADDRESS_REG_WB:
-      fprintf (file, "(%s),%ld,%u", reg_names[REGNO (addr.reg)],
+      fprintf (file, "(%s),"HOST_WIDE_INT_PRINT_DEC",%u", reg_names[REGNO (addr.reg)],
 	       INTVAL (addr.offset) >> addr.shift, addr.shift);
 	return true;
 
-- 
2.17.1

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Kito Cheng]

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

LGTM

Jin Ma <jinma@linux.alibaba.com> 於 2024年1月29日 週一 17:57 寫道：

> When using  '%ld' to print 'long long int' variable, 'fprintf' will
> produce messy output on a 32-bit system, in an incorrect instruction
> being generated, such as 'th.lwib a1,(a0),-16,4294967295'. And the
> following error occurred during compilation:
>
> Assembler messages:
> Error: improper immediate value (18446744073709551615)
>
> gcc/ChangeLog:
>
>         * config/riscv/thead.cc (th_print_operand_address): Change %ld
>         to %lld.
> ---
>  gcc/config/riscv/thead.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
> index 2955bc5f8a9..e4b8c37bc28 100644
> --- a/gcc/config/riscv/thead.cc
> +++ b/gcc/config/riscv/thead.cc
> @@ -1141,7 +1141,7 @@ th_print_operand_address (FILE *file, machine_mode
> mode, rtx x)
>        return true;
>
>      case ADDRESS_REG_WB:
> -      fprintf (file, "(%s),%ld,%u", reg_names[REGNO (addr.reg)],
> +      fprintf (file, "(%s),"HOST_WIDE_INT_PRINT_DEC",%u", reg_names[REGNO
> (addr.reg)],
>                INTVAL (addr.offset) >> addr.shift, addr.shift);
>         return true;
>
> --
> 2.17.1
>
>

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Christoph Müllner]

On Mon, Jan 29, 2024 at 1:32 PM Kito Cheng <kito.cheng@gmail.com> wrote:
>
> LGTM

I've rebased, retested (rv64+rv32) and merged this patch.

Thanks!

>
> Jin Ma <jinma@linux.alibaba.com> 於 2024年1月29日 週一 17:57 寫道：
>>
>> When using  '%ld' to print 'long long int' variable, 'fprintf' will
>> produce messy output on a 32-bit system, in an incorrect instruction
>> being generated, such as 'th.lwib a1,(a0),-16,4294967295'. And the
>> following error occurred during compilation:
>>
>> Assembler messages:
>> Error: improper immediate value (18446744073709551615)
>>
>> gcc/ChangeLog:
>>
>>         * config/riscv/thead.cc (th_print_operand_address): Change %ld
>>         to %lld.
>> ---
>>  gcc/config/riscv/thead.cc | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gcc/config/riscv/thead.cc b/gcc/config/riscv/thead.cc
>> index 2955bc5f8a9..e4b8c37bc28 100644
>> --- a/gcc/config/riscv/thead.cc
>> +++ b/gcc/config/riscv/thead.cc
>> @@ -1141,7 +1141,7 @@ th_print_operand_address (FILE *file, machine_mode mode, rtx x)
>>        return true;
>>
>>      case ADDRESS_REG_WB:
>> -      fprintf (file, "(%s),%ld,%u", reg_names[REGNO (addr.reg)],
>> +      fprintf (file, "(%s),"HOST_WIDE_INT_PRINT_DEC",%u", reg_names[REGNO (addr.reg)],
>>                INTVAL (addr.offset) >> addr.shift, addr.shift);
>>         return true;
>>
>> --
>> 2.17.1
>>

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Andreas Schwab]

On Jan 30 2024, Christoph Müllner wrote:

> retested

Nope.

../../gcc/config/riscv/thead.cc:1144:22: error: invalid suffix on literal; C++11 requires a space between literal and string macro [-Werror=literal-suffix]
 1144 |       fprintf (file, "(%s),"HOST_WIDE_INT_PRINT_DEC",%u", reg_names[REGNO (addr.reg)],
      |                      ^
cc1plus: all warnings being treated as errors
make[3]: *** [../../gcc/config/riscv/t-riscv:127: thead.o] Error 1

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Christoph Müllner]

On Sat, Feb 3, 2024 at 2:11 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Jan 30 2024, Christoph Müllner wrote:
>
> > retested
>
> Nope.

Sorry for this.
I tested for no regressions in the test suite with a cross-build and
QEMU and did not do a Werror bootstrap build.
I'll provide a fix for this later today (also breaking the line as it
is longer than needed).


>
> ../../gcc/config/riscv/thead.cc:1144:22: error: invalid suffix on literal; C++11 requires a space between literal and string macro [-Werror=literal-suffix]
>  1144 |       fprintf (file, "(%s),"HOST_WIDE_INT_PRINT_DEC",%u", reg_names[REGNO (addr.reg)],
>       |                      ^
> cc1plus: all warnings being treated as errors
> make[3]: *** [../../gcc/config/riscv/t-riscv:127: thead.o] Error 1
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Jeff Law]

On 2/5/24 05:00, Christoph Müllner wrote:
> On Sat, Feb 3, 2024 at 2:11 PM Andreas Schwab <schwab@linux-m68k.org>
> wrote:
>> 
>> On Jan 30 2024, Christoph Müllner wrote:
>> 
>>> retested
>> 
>> Nope.
> 
> Sorry for this. I tested for no regressions in the test suite with a
> cross-build and QEMU and did not do a Werror bootstrap build. I'll
> provide a fix for this later today (also breaking the line as it is
> longer than needed).
Right.  And that's pretty standard given the state of the RISC-V 
platforms.  We've got a platform here that can bootstrap in a reasonable 
amount of time, but I haven't set that up in the CI system yet.

Until such systems are common, these niggling issues are bound to show up.

It's just whitespace around the HOST_WIDE_INT_PRINT_DEC and wrapping the 
long line, right?  I've got that in my tree that's bootstrapping now.  I 
don't mind committing it later today.  But if you get to it before my 
bootstrap is done, feel free to commit as pre-approved.

jeff

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Andreas Schwab]

On Feb 05 2024, Jeff Law wrote:

> Until such systems are common, these niggling issues are bound to show up.

It won't if you do it properly: build with a cross compiler that was
built from the same source and enable -Werror.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Jeff Law]

On 2/5/24 08:08, Andreas Schwab wrote:
> On Feb 05 2024, Jeff Law wrote:
> 
>> Until such systems are common, these niggling issues are bound to show up.
> 
> It won't if you do it properly: build with a cross compiler that was
> built from the same source and enable -Werror.
We're all aware you *can* do that.  But it's never been a requirement to 
commit a patch.

jeff

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Andreas Schwab]

On Feb 05 2024, Jeff Law wrote:

> We're all aware you *can* do that.  But it's never been a requirement to
> commit a patch.

It has always been a requirement that a patch does not break bootstrap.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2] RISC-V: THEAD: Fix improper immediate value for MODIFY_DISP instruction on 32-bit systems.

[Christoph Müllner]

On Mon, Feb 5, 2024 at 3:56 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 2/5/24 05:00, Christoph Müllner wrote:
> > On Sat, Feb 3, 2024 at 2:11 PM Andreas Schwab <schwab@linux-m68k.org>
> > wrote:
> >>
> >> On Jan 30 2024, Christoph Müllner wrote:
> >>
> >>> retested
> >>
> >> Nope.
> >
> > Sorry for this. I tested for no regressions in the test suite with a
> > cross-build and QEMU and did not do a Werror bootstrap build. I'll
> > provide a fix for this later today (also breaking the line as it is
> > longer than needed).
> Right.  And that's pretty standard given the state of the RISC-V
> platforms.  We've got a platform here that can bootstrap in a reasonable
> amount of time, but I haven't set that up in the CI system yet.
>
> Until such systems are common, these niggling issues are bound to show up.
>
> It's just whitespace around the HOST_WIDE_INT_PRINT_DEC and wrapping the
> long line, right?  I've got that in my tree that's bootstrapping now.  I
> don't mind committing it later today.  But if you get to it before my
> bootstrap is done, feel free to commit as pre-approved.

Pushed:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=184978cd74f962712e813030d58edc109ad9a92d

>
> jeff