RISC-V: Fix splitter for 32-bit AND on 64-bit target.

Message ID 20190708104824.27431-1-jimw@sifive.com
State New
Headers show
Series
  • RISC-V: Fix splitter for 32-bit AND on 64-bit target.
Related show

Commit Message

Jim Wilson July 8, 2019, 10:48 a.m.
Fixes github.com/riscv/riscv-gcc issue #161.  We were accidentally using
BITS_PER_WORD to compute shift counts when we should have been using the
bitsize of the operand modes.  This was wrong when we had an SImode shift
and a 64-bit target.

Tested with 32-bit elf and 64-bit linux cross compiler builds and checks.
There were no regressions.  The new test fails without the patch and works
with the patch.

Committed.

Jim

	Andrew Waterman  <andrew@sifive.com>
	gcc/
	* config/riscv/riscv.md (lshrsi3_zero_extend_3+1): Use operands[1]
	bitsize	instead of BITS_PER_WORD.
	gcc/testsuite/
	* gcc.target/riscv/shift-shift-2.c: Add one more test.
---
 gcc/config/riscv/riscv.md                      |  5 +++--
 gcc/testsuite/gcc.target/riscv/shift-shift-2.c | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.17.1

Comments

Kito Cheng July 16, 2019, 7:42 a.m. | #1
Hi

I'd like to back port this patch to GCC 8 and 9, should I send another
patch mail or just wait ack from release manager?



On Mon, Jul 8, 2019 at 6:48 PM Jim Wilson <jimw@sifive.com> wrote:
>

> Fixes github.com/riscv/riscv-gcc issue #161.  We were accidentally using

> BITS_PER_WORD to compute shift counts when we should have been using the

> bitsize of the operand modes.  This was wrong when we had an SImode shift

> and a 64-bit target.

>

> Tested with 32-bit elf and 64-bit linux cross compiler builds and checks.

> There were no regressions.  The new test fails without the patch and works

> with the patch.

>

> Committed.

>

> Jim

>

>         Andrew Waterman  <andrew@sifive.com>

>         gcc/

>         * config/riscv/riscv.md (lshrsi3_zero_extend_3+1): Use operands[1]

>         bitsize instead of BITS_PER_WORD.

>         gcc/testsuite/

>         * gcc.target/riscv/shift-shift-2.c: Add one more test.

> ---

>  gcc/config/riscv/riscv.md                      |  5 +++--

>  gcc/testsuite/gcc.target/riscv/shift-shift-2.c | 16 ++++++++++++++--

>  2 files changed, 17 insertions(+), 4 deletions(-)

>

> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md

> index 0f4626656d6..78260fcf6fd 100644

> --- a/gcc/config/riscv/riscv.md

> +++ b/gcc/config/riscv/riscv.md

> @@ -1776,10 +1776,11 @@

>    (set (match_dup 0)

>         (lshiftrt:GPR (match_dup 0) (match_dup 2)))]

>  {

> -  operands[2] = GEN_INT (BITS_PER_WORD

> +  /* Op2 is a VOIDmode constant, so get the mode size from op1.  */

> +  operands[2] = GEN_INT (GET_MODE_BITSIZE (GET_MODE (operands[1]))

>                          - exact_log2 (INTVAL (operands[2]) + 1));

>  })

> -

> +

>  ;; Handle AND with 0xF...F0...0 where there are 32 to 63 zeros.  This can be

>  ;; split into two shifts.  Otherwise it requires 3 instructions: li, sll, and.

>  (define_split

> diff --git a/gcc/testsuite/gcc.target/riscv/shift-shift-2.c b/gcc/testsuite/gcc.target/riscv/shift-shift-2.c

> index 3f07e7776e7..10a5bb728be 100644

> --- a/gcc/testsuite/gcc.target/riscv/shift-shift-2.c

> +++ b/gcc/testsuite/gcc.target/riscv/shift-shift-2.c

> @@ -25,5 +25,17 @@ sub4 (unsigned long i)

>  {

>    return (i << 52) >> 52;

>  }

> -/* { dg-final { scan-assembler-times "slli" 4 } } */

> -/* { dg-final { scan-assembler-times "srli" 4 } } */

> +

> +unsigned int

> +sub5 (unsigned int i)

> +{

> +  unsigned int j;

> +  j = i >> 24;

> +  j = j * (1 << 24);

> +  j = i - j;

> +  return j;

> +}

> +/* { dg-final { scan-assembler-times "slli" 5 } } */

> +/* { dg-final { scan-assembler-times "srli" 5 } } */

> +/* { dg-final { scan-assembler-times "slliw" 1 } } */

> +/* { dg-final { scan-assembler-times "srliw" 1 } } */

> --

> 2.17.1

>
Richard Biener July 16, 2019, 10:51 a.m. | #2
On Tue, 16 Jul 2019, Kito Cheng wrote:

> Hi

> 

> I'd like to back port this patch to GCC 8 and 9, should I send another

> patch mail or just wait ack from release manager?


Looks OK to backport since it only affects RISC-V and that's neither
a primary nor secondary arch and this looks like a wrong-code issue.

Richard.

> 

> 

> On Mon, Jul 8, 2019 at 6:48 PM Jim Wilson <jimw@sifive.com> wrote:

> >

> > Fixes github.com/riscv/riscv-gcc issue #161.  We were accidentally using

> > BITS_PER_WORD to compute shift counts when we should have been using the

> > bitsize of the operand modes.  This was wrong when we had an SImode shift

> > and a 64-bit target.

> >

> > Tested with 32-bit elf and 64-bit linux cross compiler builds and checks.

> > There were no regressions.  The new test fails without the patch and works

> > with the patch.

> >

> > Committed.

> >

> > Jim

> >

> >         Andrew Waterman  <andrew@sifive.com>

> >         gcc/

> >         * config/riscv/riscv.md (lshrsi3_zero_extend_3+1): Use operands[1]

> >         bitsize instead of BITS_PER_WORD.

> >         gcc/testsuite/

> >         * gcc.target/riscv/shift-shift-2.c: Add one more test.

> > ---

> >  gcc/config/riscv/riscv.md                      |  5 +++--

> >  gcc/testsuite/gcc.target/riscv/shift-shift-2.c | 16 ++++++++++++++--

> >  2 files changed, 17 insertions(+), 4 deletions(-)

> >

> > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md

> > index 0f4626656d6..78260fcf6fd 100644

> > --- a/gcc/config/riscv/riscv.md

> > +++ b/gcc/config/riscv/riscv.md

> > @@ -1776,10 +1776,11 @@

> >    (set (match_dup 0)

> >         (lshiftrt:GPR (match_dup 0) (match_dup 2)))]

> >  {

> > -  operands[2] = GEN_INT (BITS_PER_WORD

> > +  /* Op2 is a VOIDmode constant, so get the mode size from op1.  */

> > +  operands[2] = GEN_INT (GET_MODE_BITSIZE (GET_MODE (operands[1]))

> >                          - exact_log2 (INTVAL (operands[2]) + 1));

> >  })

> > -

> > +

> >  ;; Handle AND with 0xF...F0...0 where there are 32 to 63 zeros.  This can be

> >  ;; split into two shifts.  Otherwise it requires 3 instructions: li, sll, and.

> >  (define_split

> > diff --git a/gcc/testsuite/gcc.target/riscv/shift-shift-2.c b/gcc/testsuite/gcc.target/riscv/shift-shift-2.c

> > index 3f07e7776e7..10a5bb728be 100644

> > --- a/gcc/testsuite/gcc.target/riscv/shift-shift-2.c

> > +++ b/gcc/testsuite/gcc.target/riscv/shift-shift-2.c

> > @@ -25,5 +25,17 @@ sub4 (unsigned long i)

> >  {

> >    return (i << 52) >> 52;

> >  }

> > -/* { dg-final { scan-assembler-times "slli" 4 } } */

> > -/* { dg-final { scan-assembler-times "srli" 4 } } */

> > +

> > +unsigned int

> > +sub5 (unsigned int i)

> > +{

> > +  unsigned int j;

> > +  j = i >> 24;

> > +  j = j * (1 << 24);

> > +  j = i - j;

> > +  return j;

> > +}

> > +/* { dg-final { scan-assembler-times "slli" 5 } } */

> > +/* { dg-final { scan-assembler-times "srli" 5 } } */

> > +/* { dg-final { scan-assembler-times "slliw" 1 } } */

> > +/* { dg-final { scan-assembler-times "srliw" 1 } } */

> > --

> > 2.17.1

> >

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imend├Ârffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG N├╝rnberg)
Jim Wilson July 16, 2019, 6 p.m. | #3
On Tue, Jul 16, 2019 at 12:42 AM Kito Cheng <kito.cheng@gmail.com> wrote:
> I'd like to back port this patch to GCC 8 and 9, should I send another

> patch mail or just wait ack from release manager?


I think it is only relevant to the gcc-9 branch.  The buggy pattern
was added June 2018, which is a while after the gcc-8 branch was made,
and not added to the gcc-8 branch because it was an optimization, not
a bug fix.

Jim

Patch

diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 0f4626656d6..78260fcf6fd 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -1776,10 +1776,11 @@ 
   (set (match_dup 0)
        (lshiftrt:GPR (match_dup 0) (match_dup 2)))]
 {
-  operands[2] = GEN_INT (BITS_PER_WORD
+  /* Op2 is a VOIDmode constant, so get the mode size from op1.  */
+  operands[2] = GEN_INT (GET_MODE_BITSIZE (GET_MODE (operands[1]))
 			 - exact_log2 (INTVAL (operands[2]) + 1));
 })
-  
+
 ;; Handle AND with 0xF...F0...0 where there are 32 to 63 zeros.  This can be
 ;; split into two shifts.  Otherwise it requires 3 instructions: li, sll, and.
 (define_split
diff --git a/gcc/testsuite/gcc.target/riscv/shift-shift-2.c b/gcc/testsuite/gcc.target/riscv/shift-shift-2.c
index 3f07e7776e7..10a5bb728be 100644
--- a/gcc/testsuite/gcc.target/riscv/shift-shift-2.c
+++ b/gcc/testsuite/gcc.target/riscv/shift-shift-2.c
@@ -25,5 +25,17 @@  sub4 (unsigned long i)
 {
   return (i << 52) >> 52;
 }
-/* { dg-final { scan-assembler-times "slli" 4 } } */
-/* { dg-final { scan-assembler-times "srli" 4 } } */
+
+unsigned int
+sub5 (unsigned int i)
+{
+  unsigned int j;
+  j = i >> 24;
+  j = j * (1 << 24);
+  j = i - j;
+  return j;
+}
+/* { dg-final { scan-assembler-times "slli" 5 } } */
+/* { dg-final { scan-assembler-times "srli" 5 } } */
+/* { dg-final { scan-assembler-times "slliw" 1 } } */
+/* { dg-final { scan-assembler-times "srliw" 1 } } */