[PATCH] bpf: fix pseudoc w regs for small modes [PR111029]

[PATCH] bpf: fix pseudoc w regs for small modes [PR111029]

[David Faust]

In the BPF pseudo-c assembly dialect, registers treated as 32-bits
rather than the full 64 in various instructions ought to be printed as
"wN" rather than "rN".  But bpf_print_register () was only doing this
for specifically SImode registers, meaning smaller modes were printed
incorrectly.

This caused assembler errors like:

  Error: unrecognized instruction `w2 =(s8)r1'

for a 32-bit sign-extending register move instruction, where the source
register is used in QImode.

Fix bpf_print_register () to print the "w" version of register when
specified by the template for any mode 32-bits or smaller.

Tested on bpf-unknown-none.

	PR target/111029

gcc/
	* config/bpf/bpf.cc (bpf_print_register): Print 'w' registers
	for any mode 32-bits or smaller, not just SImode.

gcc/testsuite/

	* gcc.target/bpf/smov-2.c: New test.
	* gcc.target/bpf/smov-pseudoc-2.c: New test.
---
 gcc/config/bpf/bpf.cc                         |  2 +-
 gcc/testsuite/gcc.target/bpf/smov-2.c         | 15 +++++++++++++++
 gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c | 15 +++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/bpf/smov-2.c
 create mode 100644 gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c

diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
index 3516b79bce4..1d0abd7fbb3 100644
--- a/gcc/config/bpf/bpf.cc
+++ b/gcc/config/bpf/bpf.cc
@@ -753,7 +753,7 @@ bpf_print_register (FILE *file, rtx op, int code)
     fprintf (file, "%s", reg_names[REGNO (op)]);
   else
     {
-      if (code == 'w' && GET_MODE (op) == SImode)
+      if (code == 'w' && GET_MODE_SIZE (GET_MODE (op)) <= 4)
 	{
 	  if (REGNO (op) == BPF_FP)
 	    fprintf (file, "w10");
diff --git a/gcc/testsuite/gcc.target/bpf/smov-2.c b/gcc/testsuite/gcc.target/bpf/smov-2.c
new file mode 100644
index 00000000000..6f3516d2385
--- /dev/null
+++ b/gcc/testsuite/gcc.target/bpf/smov-2.c
@@ -0,0 +1,15 @@
+/* Check signed 32-bit mov instructions.  */
+/* { dg-do compile } */
+/* { dg-options "-mcpu=v4 -O2" } */
+
+int
+foo (unsigned char a, unsigned short b)
+{
+  int x = (char) a;
+  int y = (short) b;
+
+  return x + y;
+}
+
+/* { dg-final { scan-assembler {movs32\t%r.,%r.,8\n} } } */
+/* { dg-final { scan-assembler {movs32\t%r.,%r.,16\n} } } */
diff --git a/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
new file mode 100644
index 00000000000..6af6cadf8df
--- /dev/null
+++ b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
@@ -0,0 +1,15 @@
+/* Check signed 32-bit mov instructions (pseudo-C asm dialect).  */
+/* { dg-do compile } */
+/* { dg-options "-mcpu=v4 -O2 -masm=pseudoc" } */
+
+int
+foo (unsigned char a, unsigned short b)
+{
+  int x = (char) a;
+  int y = (short) b;
+
+  return x + y;
+}
+
+/* { dg-final { scan-assembler {w. = \(s8\) w.\n} } } */
+/* { dg-final { scan-assembler {w. = \(s16\) w.\n} } } */
-- 
2.40.1

Re: [PATCH] bpf: fix pseudoc w regs for small modes [PR111029]

[Jose E. Marchesi]

Hello David.
Thanks for the patch.

OK.

> In the BPF pseudo-c assembly dialect, registers treated as 32-bits
> rather than the full 64 in various instructions ought to be printed as
> "wN" rather than "rN".  But bpf_print_register () was only doing this
> for specifically SImode registers, meaning smaller modes were printed
> incorrectly.
>
> This caused assembler errors like:
>
>   Error: unrecognized instruction `w2 =(s8)r1'
>
> for a 32-bit sign-extending register move instruction, where the source
> register is used in QImode.
>
> Fix bpf_print_register () to print the "w" version of register when
> specified by the template for any mode 32-bits or smaller.
>
> Tested on bpf-unknown-none.
>
> 	PR target/111029
>
> gcc/
> 	* config/bpf/bpf.cc (bpf_print_register): Print 'w' registers
> 	for any mode 32-bits or smaller, not just SImode.
>
> gcc/testsuite/
>
> 	* gcc.target/bpf/smov-2.c: New test.
> 	* gcc.target/bpf/smov-pseudoc-2.c: New test.
> ---
>  gcc/config/bpf/bpf.cc                         |  2 +-
>  gcc/testsuite/gcc.target/bpf/smov-2.c         | 15 +++++++++++++++
>  gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c | 15 +++++++++++++++
>  3 files changed, 31 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/bpf/smov-2.c
>  create mode 100644 gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
>
> diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
> index 3516b79bce4..1d0abd7fbb3 100644
> --- a/gcc/config/bpf/bpf.cc
> +++ b/gcc/config/bpf/bpf.cc
> @@ -753,7 +753,7 @@ bpf_print_register (FILE *file, rtx op, int code)
>      fprintf (file, "%s", reg_names[REGNO (op)]);
>    else
>      {
> -      if (code == 'w' && GET_MODE (op) == SImode)
> +      if (code == 'w' && GET_MODE_SIZE (GET_MODE (op)) <= 4)
>  	{
>  	  if (REGNO (op) == BPF_FP)
>  	    fprintf (file, "w10");
> diff --git a/gcc/testsuite/gcc.target/bpf/smov-2.c b/gcc/testsuite/gcc.target/bpf/smov-2.c
> new file mode 100644
> index 00000000000..6f3516d2385
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/bpf/smov-2.c
> @@ -0,0 +1,15 @@
> +/* Check signed 32-bit mov instructions.  */
> +/* { dg-do compile } */
> +/* { dg-options "-mcpu=v4 -O2" } */
> +
> +int
> +foo (unsigned char a, unsigned short b)
> +{
> +  int x = (char) a;
> +  int y = (short) b;
> +
> +  return x + y;
> +}
> +
> +/* { dg-final { scan-assembler {movs32\t%r.,%r.,8\n} } } */
> +/* { dg-final { scan-assembler {movs32\t%r.,%r.,16\n} } } */
> diff --git a/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
> new file mode 100644
> index 00000000000..6af6cadf8df
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
> @@ -0,0 +1,15 @@
> +/* Check signed 32-bit mov instructions (pseudo-C asm dialect).  */
> +/* { dg-do compile } */
> +/* { dg-options "-mcpu=v4 -O2 -masm=pseudoc" } */
> +
> +int
> +foo (unsigned char a, unsigned short b)
> +{
> +  int x = (char) a;
> +  int y = (short) b;
> +
> +  return x + y;
> +}
> +
> +/* { dg-final { scan-assembler {w. = \(s8\) w.\n} } } */
> +/* { dg-final { scan-assembler {w. = \(s16\) w.\n} } } */

Re: [PATCH] bpf: fix pseudoc w regs for small modes [PR111029]

[Richard Biener]

On Tue, Aug 15, 2023 at 9:03 PM Jose E. Marchesi via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> Hello David.
> Thanks for the patch.
>
> OK.

Picking a random patch/mail for this question - how do we maintain BPF
support for the most recent GCC release which is GCC 13?  I see the
current state in GCC 13 isn't fully able to provide upstream kernel BPF
support but GCC 14 contains some bugfixes and some new features(?).
Is it worthwhile to backport at least bugfixes while GCC 14 is still in
development even if those are not regression fixes?  Or is GCC 13 BPF
too broken to be used anyway?

Thanks,
Richard.

> > In the BPF pseudo-c assembly dialect, registers treated as 32-bits
> > rather than the full 64 in various instructions ought to be printed as
> > "wN" rather than "rN".  But bpf_print_register () was only doing this
> > for specifically SImode registers, meaning smaller modes were printed
> > incorrectly.
> >
> > This caused assembler errors like:
> >
> >   Error: unrecognized instruction `w2 =(s8)r1'
> >
> > for a 32-bit sign-extending register move instruction, where the source
> > register is used in QImode.
> >
> > Fix bpf_print_register () to print the "w" version of register when
> > specified by the template for any mode 32-bits or smaller.
> >
> > Tested on bpf-unknown-none.
> >
> >       PR target/111029
> >
> > gcc/
> >       * config/bpf/bpf.cc (bpf_print_register): Print 'w' registers
> >       for any mode 32-bits or smaller, not just SImode.
> >
> > gcc/testsuite/
> >
> >       * gcc.target/bpf/smov-2.c: New test.
> >       * gcc.target/bpf/smov-pseudoc-2.c: New test.
> > ---
> >  gcc/config/bpf/bpf.cc                         |  2 +-
> >  gcc/testsuite/gcc.target/bpf/smov-2.c         | 15 +++++++++++++++
> >  gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c | 15 +++++++++++++++
> >  3 files changed, 31 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.target/bpf/smov-2.c
> >  create mode 100644 gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
> >
> > diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
> > index 3516b79bce4..1d0abd7fbb3 100644
> > --- a/gcc/config/bpf/bpf.cc
> > +++ b/gcc/config/bpf/bpf.cc
> > @@ -753,7 +753,7 @@ bpf_print_register (FILE *file, rtx op, int code)
> >      fprintf (file, "%s", reg_names[REGNO (op)]);
> >    else
> >      {
> > -      if (code == 'w' && GET_MODE (op) == SImode)
> > +      if (code == 'w' && GET_MODE_SIZE (GET_MODE (op)) <= 4)
> >       {
> >         if (REGNO (op) == BPF_FP)
> >           fprintf (file, "w10");
> > diff --git a/gcc/testsuite/gcc.target/bpf/smov-2.c b/gcc/testsuite/gcc.target/bpf/smov-2.c
> > new file mode 100644
> > index 00000000000..6f3516d2385
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/bpf/smov-2.c
> > @@ -0,0 +1,15 @@
> > +/* Check signed 32-bit mov instructions.  */
> > +/* { dg-do compile } */
> > +/* { dg-options "-mcpu=v4 -O2" } */
> > +
> > +int
> > +foo (unsigned char a, unsigned short b)
> > +{
> > +  int x = (char) a;
> > +  int y = (short) b;
> > +
> > +  return x + y;
> > +}
> > +
> > +/* { dg-final { scan-assembler {movs32\t%r.,%r.,8\n} } } */
> > +/* { dg-final { scan-assembler {movs32\t%r.,%r.,16\n} } } */
> > diff --git a/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
> > new file mode 100644
> > index 00000000000..6af6cadf8df
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
> > @@ -0,0 +1,15 @@
> > +/* Check signed 32-bit mov instructions (pseudo-C asm dialect).  */
> > +/* { dg-do compile } */
> > +/* { dg-options "-mcpu=v4 -O2 -masm=pseudoc" } */
> > +
> > +int
> > +foo (unsigned char a, unsigned short b)
> > +{
> > +  int x = (char) a;
> > +  int y = (short) b;
> > +
> > +  return x + y;
> > +}
> > +
> > +/* { dg-final { scan-assembler {w. = \(s8\) w.\n} } } */
> > +/* { dg-final { scan-assembler {w. = \(s16\) w.\n} } } */

Re: [PATCH] bpf: fix pseudoc w regs for small modes [PR111029]

[Jose E. Marchesi]

> On Tue, Aug 15, 2023 at 9:03 PM Jose E. Marchesi via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>> Hello David.
>> Thanks for the patch.
>>
>> OK.
>
> Picking a random patch/mail for this question - how do we maintain BPF
> support for the most recent GCC release which is GCC 13?  I see the
> current state in GCC 13 isn't fully able to provide upstream kernel BPF
> support but GCC 14 contains some bugfixes and some new features(?).
> Is it worthwhile to backport at least bugfixes while GCC 14 is still in
> development even if those are not regression fixes?  Or is GCC 13 BPF
> too broken to be used anyway?

Our plan is:

1. Get git GCC and git binutils to compile all the kernel BPF selftests.
   This covers both functionality (builtins, attributes, BTF, CO-RE,
   etc) and consolidation of behavior between the GNU and llvm bpf
   ports.  We are working very hard to achieve this point and we are
   very near: functionality wise we are on-par in all components, but
   there are some bugs we are fixing.  We expect to be done in a couple
   of weeks.

2. Once the above is achieved, we plan to start doing the backports to
   released/maintained versions of both binutils and GCC so distros like
   Debian (that already package gcc-bpf) can use the toolchain.

3. Next step is to make sure the compiler generates code that can
   generally satisfy the many restrictions imposed by the kernel
   verifier, at least to a point that is practical.  This is a difficult
   general problem not specific to GCC and is shared by llvm and other
   optimizing compilers, sort of a moving target, and it is not clear at
   all how to achieve this in a general and practical way.  We have some
   ideas and have submitted a proposal to discuss this topic during this
   year's Cauldron: "The challenge of compiling for verified targets".

> Thanks,
> Richard.
>
>> > In the BPF pseudo-c assembly dialect, registers treated as 32-bits
>> > rather than the full 64 in various instructions ought to be printed as
>> > "wN" rather than "rN".  But bpf_print_register () was only doing this
>> > for specifically SImode registers, meaning smaller modes were printed
>> > incorrectly.
>> >
>> > This caused assembler errors like:
>> >
>> >   Error: unrecognized instruction `w2 =(s8)r1'
>> >
>> > for a 32-bit sign-extending register move instruction, where the source
>> > register is used in QImode.
>> >
>> > Fix bpf_print_register () to print the "w" version of register when
>> > specified by the template for any mode 32-bits or smaller.
>> >
>> > Tested on bpf-unknown-none.
>> >
>> >       PR target/111029
>> >
>> > gcc/
>> >       * config/bpf/bpf.cc (bpf_print_register): Print 'w' registers
>> >       for any mode 32-bits or smaller, not just SImode.
>> >
>> > gcc/testsuite/
>> >
>> >       * gcc.target/bpf/smov-2.c: New test.
>> >       * gcc.target/bpf/smov-pseudoc-2.c: New test.
>> > ---
>> >  gcc/config/bpf/bpf.cc                         |  2 +-
>> >  gcc/testsuite/gcc.target/bpf/smov-2.c         | 15 +++++++++++++++
>> >  gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c | 15 +++++++++++++++
>> >  3 files changed, 31 insertions(+), 1 deletion(-)
>> >  create mode 100644 gcc/testsuite/gcc.target/bpf/smov-2.c
>> >  create mode 100644 gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
>> >
>> > diff --git a/gcc/config/bpf/bpf.cc b/gcc/config/bpf/bpf.cc
>> > index 3516b79bce4..1d0abd7fbb3 100644
>> > --- a/gcc/config/bpf/bpf.cc
>> > +++ b/gcc/config/bpf/bpf.cc
>> > @@ -753,7 +753,7 @@ bpf_print_register (FILE *file, rtx op, int code)
>> >      fprintf (file, "%s", reg_names[REGNO (op)]);
>> >    else
>> >      {
>> > -      if (code == 'w' && GET_MODE (op) == SImode)
>> > +      if (code == 'w' && GET_MODE_SIZE (GET_MODE (op)) <= 4)
>> >       {
>> >         if (REGNO (op) == BPF_FP)
>> >           fprintf (file, "w10");
>> > diff --git a/gcc/testsuite/gcc.target/bpf/smov-2.c b/gcc/testsuite/gcc.target/bpf/smov-2.c
>> > new file mode 100644
>> > index 00000000000..6f3516d2385
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/bpf/smov-2.c
>> > @@ -0,0 +1,15 @@
>> > +/* Check signed 32-bit mov instructions.  */
>> > +/* { dg-do compile } */
>> > +/* { dg-options "-mcpu=v4 -O2" } */
>> > +
>> > +int
>> > +foo (unsigned char a, unsigned short b)
>> > +{
>> > +  int x = (char) a;
>> > +  int y = (short) b;
>> > +
>> > +  return x + y;
>> > +}
>> > +
>> > +/* { dg-final { scan-assembler {movs32\t%r.,%r.,8\n} } } */
>> > +/* { dg-final { scan-assembler {movs32\t%r.,%r.,16\n} } } */
>> > diff --git a/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
>> > new file mode 100644
>> > index 00000000000..6af6cadf8df
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/bpf/smov-pseudoc-2.c
>> > @@ -0,0 +1,15 @@
>> > +/* Check signed 32-bit mov instructions (pseudo-C asm dialect).  */
>> > +/* { dg-do compile } */
>> > +/* { dg-options "-mcpu=v4 -O2 -masm=pseudoc" } */
>> > +
>> > +int
>> > +foo (unsigned char a, unsigned short b)
>> > +{
>> > +  int x = (char) a;
>> > +  int y = (short) b;
>> > +
>> > +  return x + y;
>> > +}
>> > +
>> > +/* { dg-final { scan-assembler {w. = \(s8\) w.\n} } } */
>> > +/* { dg-final { scan-assembler {w. = \(s16\) w.\n} } } */