| Message ID | 20180929163607.GA9150@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: Use TImode for BLKmode values in 2 integer registers | expand |
On Sat, Sep 29, 2018 at 6:36 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > When passing and returning BLKmode values in 2 integer registers, use > 1 TImode register instead of 2 DImode registers. Otherwise, V1TImode > may be used to move and store such BLKmode values, which prevent RTL > optimizations. > > Tested on x86-64. OK for trunk? > > Thanks. > > H.J. > --- > gcc/ > > PR target/87370 > * config/i386/i386.c (construct_container): Use TImode for > BLKmode values in 2 integer registers. > > gcc/testsuite/ > > PR target/87370 > * gcc.target/i386/pr87370.c: New test. OK. Thanks, Uros. > --- > gcc/config/i386/i386.c | 17 +++++++++-- > gcc/testsuite/gcc.target/i386/pr87370.c | 39 +++++++++++++++++++++++++ > 2 files changed, 54 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/i386/pr87370.c > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > index 176cce521b7..54752513076 100644 > --- a/gcc/config/i386/i386.c > +++ b/gcc/config/i386/i386.c > @@ -7914,9 +7914,22 @@ construct_container (machine_mode mode, machine_mode orig_mode, > if (n == 2 > && regclass[0] == X86_64_INTEGER_CLASS > && regclass[1] == X86_64_INTEGER_CLASS > - && (mode == CDImode || mode == TImode) > + && (mode == CDImode || mode == TImode || mode == BLKmode) > && intreg[0] + 1 == intreg[1]) > - return gen_rtx_REG (mode, intreg[0]); > + { > + if (mode == BLKmode) > + { > + /* Use TImode for BLKmode values in 2 integer registers. */ > + exp[0] = gen_rtx_EXPR_LIST (VOIDmode, > + gen_rtx_REG (TImode, intreg[0]), > + GEN_INT (0)); > + ret = gen_rtx_PARALLEL (mode, rtvec_alloc (1)); > + XVECEXP (ret, 0, 0) = exp[0]; > + return ret; > + } > + else > + return gen_rtx_REG (mode, intreg[0]); > + } > > /* Otherwise figure out the entries of the PARALLEL. */ > for (i = 0; i < n; i++) > diff --git a/gcc/testsuite/gcc.target/i386/pr87370.c b/gcc/testsuite/gcc.target/i386/pr87370.c > new file mode 100644 > index 00000000000..c7b6295a33b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/pr87370.c > @@ -0,0 +1,39 @@ > +/* { dg-do compile { target { ! ia32 } } } */ > +/* { dg-options "-O2" } */ > + > +struct A > +{ > + int b[4]; > +}; > +struct B > +{ > + char a[12]; > + int b; > +}; > +struct C > +{ > + char a[16]; > +}; > + > +struct A > +f1 (void) > +{ > + struct A x = {}; > + return x; > +} > + > +struct B > +f2 (void) > +{ > + struct B x = {}; > + return x; > +} > + > +struct C > +f3 (void) > +{ > + struct C x = {}; > + return x; > +} > + > +/* { dg-final { scan-assembler-not "xmm" } } */ > -- > 2.17.1 >
On Sat, Sep 29, 2018 at 11:02 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Sat, Sep 29, 2018 at 6:36 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > When passing and returning BLKmode values in 2 integer registers, use > > 1 TImode register instead of 2 DImode registers. Otherwise, V1TImode > > may be used to move and store such BLKmode values, which prevent RTL > > optimizations. > > > > Tested on x86-64. OK for trunk? > > > > Thanks. > > > > H.J. > > --- > > gcc/ > > > > PR target/87370 > > * config/i386/i386.c (construct_container): Use TImode for > > BLKmode values in 2 integer registers. > > > > gcc/testsuite/ > > > > PR target/87370 > > * gcc.target/i386/pr87370.c: New test. > > OK. I'd like to backport it to release branches. Is that OK? Thanks. H.J. > Thanks, > Uros. > > > --- > > gcc/config/i386/i386.c | 17 +++++++++-- > > gcc/testsuite/gcc.target/i386/pr87370.c | 39 +++++++++++++++++++++++++ > > 2 files changed, 54 insertions(+), 2 deletions(-) > > create mode 100644 gcc/testsuite/gcc.target/i386/pr87370.c > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > > index 176cce521b7..54752513076 100644 > > --- a/gcc/config/i386/i386.c > > +++ b/gcc/config/i386/i386.c > > @@ -7914,9 +7914,22 @@ construct_container (machine_mode mode, machine_mode orig_mode, > > if (n == 2 > > && regclass[0] == X86_64_INTEGER_CLASS > > && regclass[1] == X86_64_INTEGER_CLASS > > - && (mode == CDImode || mode == TImode) > > + && (mode == CDImode || mode == TImode || mode == BLKmode) > > && intreg[0] + 1 == intreg[1]) > > - return gen_rtx_REG (mode, intreg[0]); > > + { > > + if (mode == BLKmode) > > + { > > + /* Use TImode for BLKmode values in 2 integer registers. */ > > + exp[0] = gen_rtx_EXPR_LIST (VOIDmode, > > + gen_rtx_REG (TImode, intreg[0]), > > + GEN_INT (0)); > > + ret = gen_rtx_PARALLEL (mode, rtvec_alloc (1)); > > + XVECEXP (ret, 0, 0) = exp[0]; > > + return ret; > > + } > > + else > > + return gen_rtx_REG (mode, intreg[0]); > > + } > > > > /* Otherwise figure out the entries of the PARALLEL. */ > > for (i = 0; i < n; i++) > > diff --git a/gcc/testsuite/gcc.target/i386/pr87370.c b/gcc/testsuite/gcc.target/i386/pr87370.c > > new file mode 100644 > > index 00000000000..c7b6295a33b > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/i386/pr87370.c > > @@ -0,0 +1,39 @@ > > +/* { dg-do compile { target { ! ia32 } } } */ > > +/* { dg-options "-O2" } */ > > + > > +struct A > > +{ > > + int b[4]; > > +}; > > +struct B > > +{ > > + char a[12]; > > + int b; > > +}; > > +struct C > > +{ > > + char a[16]; > > +}; > > + > > +struct A > > +f1 (void) > > +{ > > + struct A x = {}; > > + return x; > > +} > > + > > +struct B > > +f2 (void) > > +{ > > + struct B x = {}; > > + return x; > > +} > > + > > +struct C > > +f3 (void) > > +{ > > + struct C x = {}; > > + return x; > > +} > > + > > +/* { dg-final { scan-assembler-not "xmm" } } */ > > -- > > 2.17.1 > >
On Fri, Oct 5, 2018 at 1:43 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Sat, Sep 29, 2018 at 11:02 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > On Sat, Sep 29, 2018 at 6:36 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > When passing and returning BLKmode values in 2 integer registers, use > > > 1 TImode register instead of 2 DImode registers. Otherwise, V1TImode > > > may be used to move and store such BLKmode values, which prevent RTL > > > optimizations. > > > > > > Tested on x86-64. OK for trunk? > > > > > > Thanks. > > > > > > H.J. > > > --- > > > gcc/ > > > > > > PR target/87370 > > > * config/i386/i386.c (construct_container): Use TImode for > > > BLKmode values in 2 integer registers. > > > > > > gcc/testsuite/ > > > > > > PR target/87370 > > > * gcc.target/i386/pr87370.c: New test. > > > > OK. > > I'd like to backport it to release branches. Is that OK? This is a minor optimization bug, not a wrong-code bug, so unless the patch is super-safe, I think we should leave release branches unpatched. Uros > Thanks. > > H.J. > > Thanks, > > Uros. > > > > > --- > > > gcc/config/i386/i386.c | 17 +++++++++-- > > > gcc/testsuite/gcc.target/i386/pr87370.c | 39 +++++++++++++++++++++++++ > > > 2 files changed, 54 insertions(+), 2 deletions(-) > > > create mode 100644 gcc/testsuite/gcc.target/i386/pr87370.c > > > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > > > index 176cce521b7..54752513076 100644 > > > --- a/gcc/config/i386/i386.c > > > +++ b/gcc/config/i386/i386.c > > > @@ -7914,9 +7914,22 @@ construct_container (machine_mode mode, machine_mode orig_mode, > > > if (n == 2 > > > && regclass[0] == X86_64_INTEGER_CLASS > > > && regclass[1] == X86_64_INTEGER_CLASS > > > - && (mode == CDImode || mode == TImode) > > > + && (mode == CDImode || mode == TImode || mode == BLKmode) > > > && intreg[0] + 1 == intreg[1]) > > > - return gen_rtx_REG (mode, intreg[0]); > > > + { > > > + if (mode == BLKmode) > > > + { > > > + /* Use TImode for BLKmode values in 2 integer registers. */ > > > + exp[0] = gen_rtx_EXPR_LIST (VOIDmode, > > > + gen_rtx_REG (TImode, intreg[0]), > > > + GEN_INT (0)); > > > + ret = gen_rtx_PARALLEL (mode, rtvec_alloc (1)); > > > + XVECEXP (ret, 0, 0) = exp[0]; > > > + return ret; > > > + } > > > + else > > > + return gen_rtx_REG (mode, intreg[0]); > > > + } > > > > > > /* Otherwise figure out the entries of the PARALLEL. */ > > > for (i = 0; i < n; i++) > > > diff --git a/gcc/testsuite/gcc.target/i386/pr87370.c b/gcc/testsuite/gcc.target/i386/pr87370.c > > > new file mode 100644 > > > index 00000000000..c7b6295a33b > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.target/i386/pr87370.c > > > @@ -0,0 +1,39 @@ > > > +/* { dg-do compile { target { ! ia32 } } } */ > > > +/* { dg-options "-O2" } */ > > > + > > > +struct A > > > +{ > > > + int b[4]; > > > +}; > > > +struct B > > > +{ > > > + char a[12]; > > > + int b; > > > +}; > > > +struct C > > > +{ > > > + char a[16]; > > > +}; > > > + > > > +struct A > > > +f1 (void) > > > +{ > > > + struct A x = {}; > > > + return x; > > > +} > > > + > > > +struct B > > > +f2 (void) > > > +{ > > > + struct B x = {}; > > > + return x; > > > +} > > > + > > > +struct C > > > +f3 (void) > > > +{ > > > + struct C x = {}; > > > + return x; > > > +} > > > + > > > +/* { dg-final { scan-assembler-not "xmm" } } */ > > > -- > > > 2.17.1 > > >
On Sat, Oct 6, 2018 at 12:48 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Fri, Oct 5, 2018 at 1:43 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > On Sat, Sep 29, 2018 at 11:02 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > On Sat, Sep 29, 2018 at 6:36 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > > > When passing and returning BLKmode values in 2 integer registers, use > > > > 1 TImode register instead of 2 DImode registers. Otherwise, V1TImode > > > > may be used to move and store such BLKmode values, which prevent RTL > > > > optimizations. > > > > > > > > Tested on x86-64. OK for trunk? > > > > > > > > Thanks. > > > > > > > > H.J. > > > > --- > > > > gcc/ > > > > > > > > PR target/87370 > > > > * config/i386/i386.c (construct_container): Use TImode for > > > > BLKmode values in 2 integer registers. > > > > > > > > gcc/testsuite/ > > > > > > > > PR target/87370 > > > > * gcc.target/i386/pr87370.c: New test. > > > > > > OK. > > > > I'd like to backport it to release branches. Is that OK? > > This is a minor optimization bug, not a wrong-code bug, so unless the > patch is super-safe, I think we should leave release branches > unpatched. > This is a regression against GCC 6. I think we should fix it for GCC 7 and 8.
On Sat, Oct 6, 2018 at 2:22 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Sat, Oct 6, 2018 at 12:48 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > On Fri, Oct 5, 2018 at 1:43 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > On Sat, Sep 29, 2018 at 11:02 AM Uros Bizjak <ubizjak@gmail.com> wrote: > > > > > > > > On Sat, Sep 29, 2018 at 6:36 PM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > > > > > When passing and returning BLKmode values in 2 integer registers, use > > > > > 1 TImode register instead of 2 DImode registers. Otherwise, V1TImode > > > > > may be used to move and store such BLKmode values, which prevent RTL > > > > > optimizations. > > > > > > > > > > Tested on x86-64. OK for trunk? > > > > > > > > > > Thanks. > > > > > > > > > > H.J. > > > > > --- > > > > > gcc/ > > > > > > > > > > PR target/87370 > > > > > * config/i386/i386.c (construct_container): Use TImode for > > > > > BLKmode values in 2 integer registers. > > > > > > > > > > gcc/testsuite/ > > > > > > > > > > PR target/87370 > > > > > * gcc.target/i386/pr87370.c: New test. > > > > > > > > OK. > > > > > > I'd like to backport it to release branches. Is that OK? > > > > This is a minor optimization bug, not a wrong-code bug, so unless the > > patch is super-safe, I think we should leave release branches > > unpatched. > > > > This is a regression against GCC 6. I think we should fix it for > GCC 7 and 8. OK then, but please wait a couple of days for possible regressions on trunk. Thanks, Uros. > -- > H.J.
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 176cce521b7..54752513076 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -7914,9 +7914,22 @@ construct_container (machine_mode mode, machine_mode orig_mode, if (n == 2 && regclass[0] == X86_64_INTEGER_CLASS && regclass[1] == X86_64_INTEGER_CLASS - && (mode == CDImode || mode == TImode) + && (mode == CDImode || mode == TImode || mode == BLKmode) && intreg[0] + 1 == intreg[1]) - return gen_rtx_REG (mode, intreg[0]); + { + if (mode == BLKmode) + { + /* Use TImode for BLKmode values in 2 integer registers. */ + exp[0] = gen_rtx_EXPR_LIST (VOIDmode, + gen_rtx_REG (TImode, intreg[0]), + GEN_INT (0)); + ret = gen_rtx_PARALLEL (mode, rtvec_alloc (1)); + XVECEXP (ret, 0, 0) = exp[0]; + return ret; + } + else + return gen_rtx_REG (mode, intreg[0]); + } /* Otherwise figure out the entries of the PARALLEL. */ for (i = 0; i < n; i++) diff --git a/gcc/testsuite/gcc.target/i386/pr87370.c b/gcc/testsuite/gcc.target/i386/pr87370.c new file mode 100644 index 00000000000..c7b6295a33b --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr87370.c @@ -0,0 +1,39 @@ +/* { dg-do compile { target { ! ia32 } } } */ +/* { dg-options "-O2" } */ + +struct A +{ + int b[4]; +}; +struct B +{ + char a[12]; + int b; +}; +struct C +{ + char a[16]; +}; + +struct A +f1 (void) +{ + struct A x = {}; + return x; +} + +struct B +f2 (void) +{ + struct B x = {}; + return x; +} + +struct C +f3 (void) +{ + struct C x = {}; + return x; +} + +/* { dg-final { scan-assembler-not "xmm" } } */