[Bug tree-optimization/63986] New: [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[Bug tree-optimization/63986] New: [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

            Bug ID: 63986
           Summary: [5 Regression][SH] gcc.target/sh/pr51244-15.c failures
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: olegendo at gcc dot gnu.org
            Target: sh

Starting with r217496 the SH specific test cases in gcc.target/sh/pr51244-15.c
are failing.
The reason for this is that, e.g. a function such as

typedef char bool;

int
test_0 (int a, int b, int c, int* d)
{
  bool x = a == 0;
  d[2] = !x;
  return x ? b : c;
}

with -O2 -m4 compiles to:
        tst     r4,r4      <<< test against zero
        mov     #-1,r1
        negc    r1,r1
        tst     r4,r4      <<< redundant test against zero
        bt/s    .L5
        mov.l   r1,@(8,r7)
        mov     r6,r5
.L5:
        rts
        mov     r5,r0



and the expected result is:
        tst     r4,r4      <<< only one test against zero
        movt    r0
        xor     #1,r0
        bt/s    .L4
        mov.l   r0,@(8,r7)
        mov     r6,r5
.L4:
        rts
        mov     r5,r0


Before r217496 there are the following insns before the combine pass that are
used to do the test against zero and store the inverted and non-inverted test
result in a reg:

(insn 11 6 14 2 (set (reg:SI 147 t)
        (eq:SI (reg/v:SI 167 [ a ])
            (const_int 0 [0]))) sh_tmp.cpp:55 17 {cmpeqsi_t}
     (expr_list:REG_DEAD (reg/v:SI 167 [ a ])
        (nil)))
(insn 14 11 15 2 (set (reg:SI 163 [ D.1480 ])
        (zero_extend:SI (subreg:QI (reg:SI 147 t) 0))) sh_tmp.cpp:55 398 {movt}
     (expr_list:REG_DEAD (reg:SI 147 t)
        (nil)))
(insn 15 14 16 2 (set (reg:SI 173)
        (xor:SI (reg:SI 163 [ D.1480 ])
            (const_int 1 [0x1]))) sh_tmp.cpp:56 138 {*xorsi3_compact}
     (nil))
(insn 16 15 17 2 (set (reg:SI 174)
        (zero_extend:SI (subreg:QI (reg:SI 173) 0))) sh_tmp.cpp:56 224
{*zero_extendqisi2_compact}
     (expr_list:REG_DEAD (reg:SI 173)
        (nil)))

combine then uses the following pattern from sh.md:

Trying 14 -> 15:
Successfully matched this instruction:
(parallel [
        (set (reg:SI 173)
            (xor:SI (reg:SI 147 t)
                (const_int 1 [0x1])))
        (set (reg:SI 163 [ D.1480 ])
            (zero_extend:SI (subreg:QI (reg:SI 147 t) 0)))
    ])

which is then split into the appropriate/expected insn sequence.


After r217496 the insns before combine look like:

(insn 11 6 12 2 (set (reg:SI 147 t)
        (eq:SI (reg/v:SI 166 [ a ])
            (const_int 0 [0]))) sh_tmp.cpp:56 17 {cmpeqsi_t}
     (nil))

(insn 13 12 14 2 (parallel [
            (set (reg:SI 171 [ D.1480 ])
                (xor:SI (reg:SI 147 t)
                    (const_int 1 [0x1])))
            (set (reg:SI 147 t)
                (const_int 1 [0x1]))
            (use (reg:SI 172))
        ]) sh_tmp.cpp:56 400 {movrt_negc}
     (expr_list:REG_DEAD (reg:SI 172)
        (expr_list:REG_UNUSED (reg:SI 147 t)
            (nil))))

(insn 15 14 7 2 (set (reg:SI 147 t)
        (eq:SI (reg/v:SI 166 [ a ])
            (const_int 0 [0]))) sh_tmp.cpp:57 17 {cmpeqsi_t}
     (expr_list:REG_DEAD (reg/v:SI 166 [ a ])
        (nil)))

and combine does not try the parallel pattern above.
I've briefly tried working around the issue with some combine bridge patterns,
but without any success.

A possible solution could be adding some extra code to the movrt_negc splitter
which looks at the surrounding insn and splits into a movt-xor sequence if
required.  However, I'd like to wait a bit until other pending combine patches
have been installed.

[Bug tree-optimization/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-11-20
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
   Target Milestone|---                         |5.0
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think you were mostly lucky -- 4.9 expands from

  <bb 2>:
  _3 = a_2(D) == 0;
  _13 = ~_3;
  _7 = (int) _13;
  MEM[(int *)d_5(D) + 8B] = _7;
  if (_3 != 0)

which it reaches in a very weird way through several optimization passes
while trunk now has

  <bb 2>:
  _7 = a_2(D) != 0;
  _8 = (int) _7;
  MEM[(int *)d_5(D) + 8B] = _8;
  if (a_2(D) == 0)

which has un-CSEd the a_2 == 0 comparison due to a bug I introduced.

[Bug tree-optimization/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, x86 code generation is the same:

test_0:
.LFB0:
        .cfi_startproc
        xorl    %eax, %eax
        testl   %edi, %edi
        setne   %al
        movl    %eax, 8(%rcx)
        movl    %edx, %eax
        cmove   %esi, %eax
        ret

so somehow it manages to fix things up again (maybe through ifcvt).

[Bug tree-optimization/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

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

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I suppose RTL CSE cannot CSE flag register sets...?

Yes, it can, if you define

 -- Target Hook: bool TARGET_FIXED_CONDITION_CODE_REGS (unsigned int
          *P1, unsigned int *P2)
     On targets which do not use `(cc0)', and which use a hard register
     rather than a pseudo-register to hold condition codes, the regular
     CSE passes are often not able to identify cases in which the hard
     register is set to a common value.  Use this hook to enable a
     small pass which optimizes such cases.  This hook should return
     true to enable this pass, and it should set the integers to which
     its arguments point to the hard register numbers used for
     condition codes.  When there is only one such register, as is true
     on most systems, the integer pointed to by P2 should be set to
     `INVALID_REGNUM'.

[Bug tree-optimization/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[ppalka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #5 from ppalka at gcc dot gnu.org ---
(In reply to Richard Biener from comment #3)
> Ok, now already existing forwprop code gets fed with
> 
>   <bb 2>:
>   _3 = a_2(D) == 0;
>   x_4 = (char) _3;
>   _7 = ~_3;
>   _8 = (int) _7;
>   MEM[(int *)d_5(D) + 8B] = _8;
>   if (x_4 != 0)
> 
> where we now in the first forwprop pass identified the opportunity
> to use ~_3 instead of x_4 == 0 thus x_4 is now no longer multi-use.
> This makes us optimize if (x_4 != 0) to if (_3 != 0) which we
> re-optimize in fold_gimple_cond now to '_3' and then of course
> if (_3 != 0) (err, and we return "changed"....) which means we
> now propagate _again_ via forward_propagate_into_gimple_cond
> which now specifically allows aggressive forwarding of compares,
> bypassing single-use restrictions.  See
> 
> 2014-11-16  Patrick Palka  <ppalka@gcc.gnu.org>
> 
>         PR middle-end/63790
>         * tree-ssa-forwprop.c (forward_propagate_into_comparison_1):
>         Always combine comparisons or conversions from booleans.
> 
> thus me fixing my "mistake" does not help anymore.

I worried that such an issue might pop up...

If you think my patch should be reverted, note that there is a more benign
version of the patch here
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00963.html (which still fixes the
test case I was originally interested in fixing).

[Bug tree-optimization/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 20 Nov 2014, ppalka at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986
> 
> --- Comment #5 from ppalka at gcc dot gnu.org ---
> (In reply to Richard Biener from comment #3)
> > Ok, now already existing forwprop code gets fed with
> > 
> >   <bb 2>:
> >   _3 = a_2(D) == 0;
> >   x_4 = (char) _3;
> >   _7 = ~_3;
> >   _8 = (int) _7;
> >   MEM[(int *)d_5(D) + 8B] = _8;
> >   if (x_4 != 0)
> > 
> > where we now in the first forwprop pass identified the opportunity
> > to use ~_3 instead of x_4 == 0 thus x_4 is now no longer multi-use.
> > This makes us optimize if (x_4 != 0) to if (_3 != 0) which we
> > re-optimize in fold_gimple_cond now to '_3' and then of course
> > if (_3 != 0) (err, and we return "changed"....) which means we
> > now propagate _again_ via forward_propagate_into_gimple_cond
> > which now specifically allows aggressive forwarding of compares,
> > bypassing single-use restrictions.  See
> > 
> > 2014-11-16  Patrick Palka  <ppalka@gcc.gnu.org>
> > 
> >         PR middle-end/63790
> >         * tree-ssa-forwprop.c (forward_propagate_into_comparison_1):
> >         Always combine comparisons or conversions from booleans.
> > 
> > thus me fixing my "mistake" does not help anymore.
> 
> I worried that such an issue might pop up...
> 
> If you think my patch should be reverted, note that there is a more benign
> version of the patch here
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg00963.html (which still fixes the
> test case I was originally interested in fixing).

No, I think that conceptually aggressive forwarding into if conditions
is wanted.  I believe it's not impossible to fix the very special
cases that might pop up - usually it will help code generation.

Richard.

[Bug tree-optimization/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #7 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #1)
> I think you were mostly lucky

And exactly that's why I'd better not rely on combine dreaming up the parallel
pattern if I want to control the instruction (sequence) selection in a more
reliable way.

I didn't mean to file this PR as tree-optimization, but rather as a target
issue.

[Bug target/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |target

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Right.  Let's put it back to target land then.  I think what the tree level
does
is reasonable.

[Bug target/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #9 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Created attachment 34073
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34073&action=edit
A possible patch

[Bug target/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #10 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Sat Nov 22 15:06:34 2014
New Revision: 217968

URL: https://gcc.gnu.org/viewcvs?rev=217968&root=gcc&view=rev
Log:
gcc/
    PR target/63986
    PR target/51244
    * config/sh/sh.c (sh_is_logical_t_store_expr,
    sh_try_omit_signzero_extend): Use rtx_insn* for insn argument.
    (sh_split_movrt_negc_to_movt_xor): New function.
    (sh_find_set_of_reg): Move to ...
    * config/sh/sh-protos.h (sh_find_set_of_reg): ... here and convert
    to template function.
    (set_of_reg): Use rtx_insn* for insn member.
    (sh_is_logical_t_store_expr, sh_try_omit_signzero_extend): Use
    rtx_insn* for insn argument.
    * config/sh/sh.md (movrt_negc, *movrt_negc): Split into movt-xor
    sequence using new sh_split_movrt_negc_to_movt_xor function.
    (movrt_xor): Allow also for SH2A.
    (*movt_movrt): Delete insns and splits.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh-protos.h
    trunk/gcc/config/sh/sh.c
    trunk/gcc/config/sh/sh.md

[Bug target/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #11 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #10)
> Author: olegendo
> Date: Sat Nov 22 15:06:34 2014
> New Revision: 217968
> 
> URL: https://gcc.gnu.org/viewcvs?rev=217968&root=gcc&view=rev

Hm, I think the patch is missing some additional checks on the operands of the
removed comparison insn, which might result in wrong code.  Please leave this
PR open for now.

[Bug target/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #12 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Mon Dec  1 06:50:06 2014
New Revision: 218200

URL: https://gcc.gnu.org/viewcvs?rev=218200&root=gcc&view=rev
Log:
gcc/
    PR target/63986
    PR target/51244
    * config/sh/sh.c (sh_unspec_insn_p,
    sh_insn_operands_modified_between_p): New functions.
    (sh_split_movrt_negc_to_movt_xor): Do not delete insn if its operands
    are modified or if it has side effects, may trap or is volatile.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh.c

[Bug target/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed?

[Bug target/63986] [5 Regression][SH] gcc.target/sh/pr51244-15.c failures

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63986

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

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

--- Comment #14 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #13)
> Fixed?

Let's assume so.