[Bug target/52483] New: SH Target: Loads from volatile memory leave redundant sign/zero extensions

[Bug target/52483] New: SH Target: Loads from volatile memory leave redundant sign/zero extensions

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52483

             Bug #: 52483
           Summary: SH Target: Loads from volatile memory leave redundant
                    sign/zero extensions
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: olegendo@gcc.gnu.org
                CC: kkojima@gcc.gnu.org
            Target: sh*-*-*


The following cases seem to happen at any optimization level:

int test_0 (volatile char* x)
{
  return *x == 5;
}

mov.b   @r4,r0  ! 6     *extendqisi2_compact/2    [length = 2]
exts.b  r0,r0   ! 7     *extendqisi2_compact/1    [length = 2]
cmp/eq  #5,r0   ! 8     cmpeqsi_t/2    [length = 2]
rts             ! 27    *return_i    [length = 2]
movt    r0      ! 14    movsi_ie/10    [length = 2]


int test_1 (volatile unsigned char* x)
{
  return *x == 0x80;
}

mov.b   @r4,r0    ! 6   *extendqisi2_compact/2    [length = 2]
exts.b  r0,r0     ! 8   *extendqisi2_compact/1    [length = 2]
cmp/eq  #-128,r0  ! 9   cmpeqsi_t/2    [length = 2]
rts               ! 28  *return_i    [length = 2]
movt    r0      ! 15  movsi_ie/10    [length = 2]


The same also happens for HImode.
Maybe a few peepholes would help here?

Using built-in specs.
COLLECT_GCC=sh-elf-gcc
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/sh-elf/4.8.0/lto-wrapper
Target: sh-elf
Configured with: ../gcc-trunk/configure --target=sh-elf --prefix=/usr/local
--enable-languages=c,c++ --enable-multilib --disable-libssp --disable-nls
--disable-werror --enable-lto --with-newlib --with-gnu-as --with-gnu-ld
--with-system-zlib
Thread model: single
gcc version 4.8.0 20120304 (experimental) (GCC)

[Bug target/52483] SH Target: Loads from volatile memory leave redundant sign/zero extensions

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52483

--- Comment #1 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-03-05 05:33:39 UTC ---
(In reply to comment #0)
> Maybe a few peepholes would help here?

Sure.  Peephole looks to be reasonable for this.

[Bug target/52483] SH Target: Loads from volatile memory leave redundant sign/zero extensions

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52483

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |olegendo at gcc dot gnu.org

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> 2012-09-17 23:05:23 UTC ---
(In reply to comment #1)
> (In reply to comment #0)
> > Maybe a few peepholes would help here?
> 
> Sure.  Peephole looks to be reasonable for this.

I've investigated a little bit regarding this issue.  First I tried to beat it
with peepholes, but the actual problem is that in the load/store expanders the
predicates use the 'nonimmediate_operand' and/or 'general_operand' functions
from recog.c, which reject volatile mems, because its 'volatile_ok' var is '0'
during RTL expansion.

As a consequence, only simple single-register addressing modes are allowed for
volatile mems.  Except for displacement address modes, since those have
explicit mem patterns, which would match during expansion.

In recog.c there is a comment:

/* Nonzero means allow operands to be volatile.
   This should be 0 if you are generating rtl, such as if you are calling
   the functions in optabs.c and expmed.c (most of the time).
   This should be 1 if all valid insns need to be recognized,
   such as in reginfo.c and final.c and reload.c.

   init_recog and init_recog_no_volatile are responsible for setting this.  */

int volatile_ok;


And in function.c:

void
expand_function_start (tree subr)
{
  /* Make sure volatile mem refs aren't considered
     valid operands of arithmetic insns.  */
  init_recog_no_volatile ();


If that is the only reason for rejecting volatile mems, then I think it would
be OK to match volatile mems in the load/store expanders for SH, since there
are no arithmetic insns that take mems anyway (except for some GBR byte mem
modification insns, which aren't used in the backend).
Kaz, does this make sense?

[Bug target/52483] SH Target: Loads from volatile memory leave redundant sign/zero extensions

[kkojima at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52483

--- Comment #3 from Kazumoto Kojima <kkojima at gcc dot gnu.org> 2012-09-18 12:25:52 UTC ---
(In reply to comment #2)
> If that is the only reason for rejecting volatile mems, then I think it would
> be OK to match volatile mems in the load/store expanders for SH, since there
> are no arithmetic insns that take mems anyway (except for some GBR byte mem
> modification insns, which aren't used in the backend).
> Kaz, does this make sense?

Yep, though I'm not sure whether that is the only reason or not.
Please go ahead and see the effect.

[Bug target/52483] SH Target: Loads from volatile memory leave redundant sign/zero extensions

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52483

--- Comment #4 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Loads from volatile mems have been fixed on 4.9 trunk.
While working on it I noticed that stores to volatile mems have basically the
same issue.  I'll try to come up with a fix for that, too.

http://gcc.gnu.org/viewcvs/gcc?view=revision&revision=200350

    PR target/52483
    * config/sh/predicates.md (general_extend_operand): Invoke
    general_movsrc_operand for memory operands.
    (general_movsrc_operand): Allow reg+reg addressing, do not use
    general_operand for memory operands.

    PR target/52483
    * gcc.target/sh/pr52483-1.c: New.
    * gcc.target/sh/pr52483-2.c: New.
    * gcc.target/sh/pr52483-3.c: New.
    * gcc.target/sh/pr52483-4.c: New.
    * gcc.target/sh/pr52483-5.c: New.

[Bug target/52483] SH Target: Loads from volatile memory leave redundant sign/zero extensions

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52483

--- Comment #5 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Sat Oct 26 22:07:37 2013
New Revision: 204097

URL: http://gcc.gnu.org/viewcvs?rev=204097&root=gcc&view=rev
Log:
    PR target/52483
    * config/sh/predicates.md (general_movdst_operand): Allow reg+reg
    addressing, do not use general_operand for memory operands.

    PR target/52483
    * gcc.target/sh/pr52483-1.c: Add tests for memory stores.
    * gcc.target/sh/pr52483-2.c: Likewise.
    * gcc.target/sh/pr52483-3.c: Likewise.
    * gcc.target/sh/pr52483-4.c: Likewise.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/predicates.md
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/sh/pr52483-1.c
    trunk/gcc/testsuite/gcc.target/sh/pr52483-2.c
    trunk/gcc/testsuite/gcc.target/sh/pr52483-3.c
    trunk/gcc/testsuite/gcc.target/sh/pr52483-4.c

[Bug target/52483] SH Target: Loads from volatile memory leave redundant sign/zero extensions

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52483

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #6 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Fixed on 4.9.