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. | expand |
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 >
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 > > >
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
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 } } */