public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2
@ 2013-09-23 21:50 olegendo at gcc dot gnu.org
  2013-10-05 20:22 ` [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and cc use olegendo at gcc dot gnu.org
                   ` (16 more replies)
  0 siblings, 17 replies; 18+ messages in thread
From: olegendo at gcc dot gnu.org @ 2013-09-23 21:50 UTC (permalink / raw)
  To: gcc-bugs

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

            Bug ID: 58517
           Summary: [SH] wrong code with subc and movsicc
                    (-mpretend-cmove) after ce2
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: olegendo at gcc dot gnu.org
            Target: sh*-*-*

While working on something else, I've noticed the following sequence in the
CSiBE set in the file zlib-1.1.4/infblock.s, compiling with -m4-single -ml -O2
-mpretend-cmove (trunk rev 201978):

.L257:
    sett        ! 1740    sett    [length = 2]
    mov    r2,r13    ! 2180    movsi_ie/2    [length = 2]
    sub    r5,r1    ! 1588    *subsi3_internal    [length = 2]
    subc    r5,r13    ! 1741    *subc    [length = 2]
    bt    0f    ! 1589    *movsicc_t_false/1    [length = 4]
    mov    r1,r13
0:
    tst    r13,r13    ! 435    cmpeqsi_t/1    [length = 2]
    bf    .L67    ! 436    *cbranch_t    [length = 2]
    bra    .L124
    nop        ! 2774    jump_compact    [length = 4]

The subc insn clobbers the T_REG and the following 'bt 0f' just looks wrong.
It seems that something goes wrong and the insn that does the test before
*movsicc_t_false gets eliminated because the sett-subc sequence is placed in
between.
After the combine pass the sequence looks like:

(note 422 421 423 45 [bb 45] NOTE_INSN_BASIC_BLOCK)
(insn 423 422 432 45 (set (reg:SI 147 t)
        (gtu:SI (reg/f:SI 424 [ D.2926 ])
            (reg/v/f:SI 189 [ q ]))) infblock.c:202 34 {cmpgtusi_t}
     (nil))
(insn 432 423 424 45 (set (reg/v:SI 190 [ n ])
        (minus:SI (reg/v/f:SI 423 [ q ])
            (reg/v/f:SI 189 [ q ]))) infblock.c:202 76 {*subsi3_internal}
     (expr_list:REG_DEAD (reg/v/f:SI 423 [ q ])
        (nil)))
(jump_insn 424 432 425 45 (set (pc)
        (if_then_else (eq (reg:SI 147 t)
                (const_int 0 [0]))
            (label_ref:SI 433)
            (pc))) infblock.c:202 295 {*cbranch_t}
     (expr_list:REG_DEAD (reg:SI 147 t)
        (expr_list:REG_BR_PROB (const_int 5000 [0x1388])
            (nil)))
 -> 433)
(note 425 424 426 46 [bb 46] NOTE_INSN_BASIC_BLOCK)
(note 426 425 427 46 NOTE_INSN_DELETED)
(insn 427 426 433 46 (parallel [
            (set (reg/v:SI 190 [ n ])
                (plus:SI (not:SI (reg/v/f:SI 189 [ q ]))
                    (reg/f:SI 424 [ D.2926 ])))
            (clobber (reg:SI 147 t))
        ]) infblock.c:202 74 {*subc}
     (expr_list:REG_UNUSED (reg:SI 147 t)
        (expr_list:REG_DEAD (reg/f:SI 424 [ D.2926 ])
            (nil))))
(code_label 433 427 434 47 70 "" [1 uses])


After the following pass ce2 it becomes:

(note 422 421 423 45 [bb 45] NOTE_INSN_BASIC_BLOCK)
(insn 423 422 432 45 (set (reg:SI 147 t)
        (gtu:SI (reg/f:SI 424 [ D.2926 ])
            (reg/v/f:SI 189 [ q ]))) infblock.c:202 34 {cmpgtusi_t}
     (nil))
(insn 432 423 1587 45 (set (reg/v:SI 190 [ n ])
        (minus:SI (reg/v/f:SI 423 [ q ])
            (reg/v/f:SI 189 [ q ]))) infblock.c:202 76 {*subsi3_internal}
     (expr_list:REG_DEAD (reg/v/f:SI 423 [ q ])
        (nil)))
(insn 1587 432 1588 45 (parallel [
            (set (reg:SI 945)
                (plus:SI (not:SI (reg/v/f:SI 189 [ q ]))
                    (reg/f:SI 424 [ D.2926 ])))
            (clobber (reg:SI 147 t))
        ]) infblock.c:202 74 {*subc}
     (nil))
(insn 1588 1587 1589 45 (set (reg:SI 946)
        (minus:SI (reg/v/f:SI 423 [ q ])
            (reg/v/f:SI 189 [ q ]))) infblock.c:202 76 {*subsi3_internal}
     (nil))
(insn 1589 1588 435 45 (set (reg/v:SI 190 [ n ])
        (if_then_else:SI (eq (reg:SI 147 t)
                (const_int 0 [0]))
            (reg:SI 946)
            (reg:SI 945))) infblock.c:202 54 {*movsicc_t_false}
     (nil))
(insn 435 1589 436 45 (set (reg:SI 147 t)
        (eq:SI (reg/v:SI 190 [ n ])
            (const_int 0 [0]))) infblock.c:202 17 {cmpeqsi_t}
     (nil))
(jump_insn 436 435 1378 45 (set (pc)
        (if_then_else (eq (reg:SI 147 t)
                (const_int 0 [0]))
            (label_ref 501)
            (pc))) infblock.c:202 295 {*cbranch_t}
     (expr_list:REG_DEAD (reg:SI 147 t)
        (expr_list:REG_BR_PROB (const_int 10000 [0x2710])
            (nil)))
 -> 501)

which is wrong.  I haven't checked whether it also happens on current trunk.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and cc use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
@ 2013-10-05 20:22 ` olegendo at gcc dot gnu.org
  2013-10-05 21:16 ` [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use olegendo at gcc dot gnu.org
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: olegendo at gcc dot gnu.org @ 2013-10-05 20:22 UTC (permalink / raw)
  To: gcc-bugs

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-10-05
          Component|target                      |rtl-optimization
            Summary|[SH] wrong code with subc   |icvt (after combine) puts
                   |and movsicc                 |ccreg clobbering insn
                   |(-mpretend-cmove) after ce2 |between ccset insn and cc
                   |                            |use
     Ever confirmed|0                           |1

--- Comment #1 from Oleg Endo <olegendo at gcc dot gnu.org> ---
This seems to be a bug in ifcvt.c

Reduced test cases:

int
test_subc (int q, int read, int end)
{
  return q < read ? (read - q - 1) : (end - q);
}

int
test_addc (int q, int read, int end)
{
  return q < read ? (read + q + 1) : (end + q);
}

compiled with -O2 -m4 -mpretend-cmove:

_test_subc:
        sett
        mov     r5,r0
        sub     r4,r6
        subc    r4,r0   ! 36    *subc
        bf      0f      ! 34    *movsicc_t_true/1
        mov     r6,r0
0:
        rts    
        nop

_test_addc:
        sett
        mov     r4,r0
        add     r4,r6
        addc    r5,r0   ! 34    *addc
        bf      0f      ! 32    *movsicc_t_true/1
        mov     r6,r0
0:
        rts    
        nop

In both above cases the comparison 'q < read' is eliminated, because ifcvt
moves the T_REG clobbering subc/addc insn between the 'q < read' comparison
(sets T_REG) and the conditional branch (uses T_REG).


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
  2013-10-05 20:22 ` [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and cc use olegendo at gcc dot gnu.org
@ 2013-10-05 21:16 ` olegendo at gcc dot gnu.org
  2013-10-05 22:17 ` olegendo at gcc dot gnu.org
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: olegendo at gcc dot gnu.org @ 2013-10-05 21:16 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> ---
The problem also happens on 4.8.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
  2013-10-05 20:22 ` [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and cc use olegendo at gcc dot gnu.org
  2013-10-05 21:16 ` [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use olegendo at gcc dot gnu.org
@ 2013-10-05 22:17 ` olegendo at gcc dot gnu.org
  2013-10-05 23:23 ` steven at gcc dot gnu.org
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: olegendo at gcc dot gnu.org @ 2013-10-05 22:17 UTC (permalink / raw)
  To: gcc-bugs

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

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

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

--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> ---
The problematic transformation happens in ifcvt.c (noce_try_cmove_arith).
I've only briefly looked over that code and can think of the following options
how to fix the issue:

1) Check whether insn_a and insn_b modify the src reg (ccreg) of the
conditional branch.  If they do, don't do the transformation.
On SH, the resulting code would be the same as not specifying -mpretend-cmove:

        cmp/ge  r5,r4
        mov     r6,r0
        bf/s    .L6
        sub     r4,r0
        rts
        nop
.L6:
        sett
        mov     r5,r0
        rts
        subc    r4,r0

2) Check whether insn_a and insn_b modify the src reg (ccreg) of the
conditional branch.  If they do, try to move the insn that sets the ccreg
before the conditional branch, _after_ the emitted insn_a and insn_b and right
before the conditional move.
On SH, the resulting code would look something like this:

        sett
        mov     r5,r0
        sub     r4,r6
        subc    r4,r0
        cmp/ge  r5,r4
        bf      0f
        mov     r6,r0
0:
        rts
        nop

Steven, since you're RTL optimizers reviewer, could you please provide some
feedback regarding the matter?


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (2 preceding siblings ...)
  2013-10-05 22:17 ` olegendo at gcc dot gnu.org
@ 2013-10-05 23:23 ` steven at gcc dot gnu.org
  2015-09-11 11:12 ` [Bug rtl-optimization/58517] ifcvt " ktkachov at gcc dot gnu.org
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: steven at gcc dot gnu.org @ 2013-10-05 23:23 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #4 from Steven Bosscher <steven at gcc dot gnu.org> ---
So if I understand correctly, this is what happens (sorry, reading
LIPSy RTL is still just too unnatural to me! ;-))

before ifcvt-after-combine:
r147:=(r424>r178)
r190:=r423-r189
if (r147==0) goto L433
{r190:=(~r189)+r424; r147:=X}
L433:

after ifcvt-after-combine:
r147:=(r424>r189)
r190:=r423-r189
{r945:=(~r189)+r424; r147:=X}
r946:=r423-r189
r190:=(r147==0) ? r946:r945      // but r147 was clobbered!
r147:=(r190==0)
if (r147==0) goto L501


This is noce_try_cmove_arith:

  /* if (test) x = a + b; else x = c - d;
     => y = a + b;
        x = c - d;
        if (test)
          x = y;
  */

with "(test)" being clobbered by the assignment to "y" or "x" on the
code after the transformation has been applied.  noce_try_cmove_arith
does not expect that the arithmetic insns emitted to load vtrue or
vfalse into general operands may clobber the condition.  There should
be tests like this:

Index: ifcvt.c
===================================================================
--- ifcvt.c     (revision 203224)
+++ ifcvt.c     (working copy)
@@ -1573,6 +1573,8 @@ noce_try_cmove_arith (struct noce_if_info *if_info
                         optimize_bb_for_speed_p (BLOCK_FOR_INSN (insn_a)));
       if (insn_cost == 0 || insn_cost > COSTS_N_INSNS (if_info->branch_cost))
        return FALSE;
+      if (modified_in_p (if_info->cond, insn_a))
+       return FALSE;
     }
   else
     insn_cost = 0;
@@ -1584,6 +1586,8 @@ noce_try_cmove_arith (struct noce_if_info *if_info
                          optimize_bb_for_speed_p (BLOCK_FOR_INSN (insn_b)));
       if (insn_cost == 0 || insn_cost > COSTS_N_INSNS (if_info->branch_cost))
         return FALSE;
+      if (modified_in_p (if_info->cond, insn_b))
+       return FALSE;
     }

   /* Possibly rearrange operands to make things come out more natural.  */



That's option 1 of comment #3.  Option 2 is only viable if the insn
computing "cond" is nearby (i.e. immediately above insn_a / insn_b),
otherwise it'll be necessary to solve a code motion dataflow problem.
Within one basic block that still would be doable (modified_between_p),
but I doubt it's worth the trouble.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (3 preceding siblings ...)
  2013-10-05 23:23 ` steven at gcc dot gnu.org
@ 2015-09-11 11:12 ` ktkachov at gcc dot gnu.org
  2023-06-02  1:38 ` pinskia at gcc dot gnu.org
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: ktkachov at gcc dot gnu.org @ 2015-09-11 11:12 UTC (permalink / raw)
  To: gcc-bugs

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

ktkachov at gcc dot gnu.org changed:

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

--- Comment #6 from ktkachov at gcc dot gnu.org ---
(In reply to Oleg Endo from comment #5)
> Kyrill, since you are currently working on ifcvt related things, could you
> maybe have a look at this issue?  I haven't checked it in a while, so I
> don't know if it's still a problem.

Huh, I wasn't aware of this BZ.
This looks eerily similar to what I think is the root cause of PR 67481, which
I'm working on now.

After I'm done with PR 67481 it would be interesting to have a look whether
they are indeed the same issue


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (4 preceding siblings ...)
  2015-09-11 11:12 ` [Bug rtl-optimization/58517] ifcvt " ktkachov at gcc dot gnu.org
@ 2023-06-02  1:38 ` pinskia at gcc dot gnu.org
  2023-06-02  1:43 ` pinskia at gcc dot gnu.org
                   ` (10 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-02  1:38 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55237
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55237&action=edit
Patch which I think will fix this

This is option 1 of comment #3 though with an updated version.
I have not tested this at all but I think it will work.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (5 preceding siblings ...)
  2023-06-02  1:38 ` pinskia at gcc dot gnu.org
@ 2023-06-02  1:43 ` pinskia at gcc dot gnu.org
  2023-06-02  1:44 ` pinskia at gcc dot gnu.org
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-02  1:43 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The reason why r6-3654-g6b7e867187889 didn't fix this case is because it was
not looking into clobbers only the set side. 

Note the conditional in my patch should have been
if (reg_overlap_mentioned_p (DF_REF_REG (def), cond))

Maybe it should be bb_valid_for_noce_process_p instead, I am not 100% sure on
that.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (6 preceding siblings ...)
  2023-06-02  1:43 ` pinskia at gcc dot gnu.org
@ 2023-06-02  1:44 ` pinskia at gcc dot gnu.org
  2023-06-02  7:28 ` pinskia at gcc dot gnu.org
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-02  1:44 UTC (permalink / raw)
  To: gcc-bugs

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2013-10-05 00:00:00         |2023-6-1

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Note the wrong code can be still reproduced in GCC 13 too.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (7 preceding siblings ...)
  2023-06-02  1:44 ` pinskia at gcc dot gnu.org
@ 2023-06-02  7:28 ` pinskia at gcc dot gnu.org
  2023-06-02  7:57 ` pinskia at gcc dot gnu.org
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-02  7:28 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #9)
> The reason why r6-3654-g6b7e867187889 didn't fix this case is because it was
> not looking into clobbers only the set side. 
> 
> Note the conditional in my patch should have been
> if (reg_overlap_mentioned_p (DF_REF_REG (def), cond))
> 
> Maybe it should be bb_valid_for_noce_process_p instead, I am not 100% sure
> on that.

This does not fix the issue as (In reply to Andrew Pinski from comment #9)
> The reason why r6-3654-g6b7e867187889 didn't fix this case is because it was
> not looking into clobbers only the set side. 
> 
> Note the conditional in my patch should have been
> if (reg_overlap_mentioned_p (DF_REF_REG (def), cond))
> 
> Maybe it should be bb_valid_for_noce_process_p instead, I am not 100% sure
> on that.

This patch is not enough. Though the patch in comment #4 is not enough any more
either.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (8 preceding siblings ...)
  2023-06-02  7:28 ` pinskia at gcc dot gnu.org
@ 2023-06-02  7:57 ` pinskia at gcc dot gnu.org
  2023-06-03  8:58 ` olegendo at gcc dot gnu.org
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-02  7:57 UTC (permalink / raw)
  To: gcc-bugs

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55237|0                           |1
        is obsolete|                            |

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55239
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55239&action=edit
Patch which does work on this

Though, I need to double to make sure it works for other cases still.
sh is the case where we have a non-CC mode register for the flags which is why
this was harder than other targets.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (9 preceding siblings ...)
  2023-06-02  7:57 ` pinskia at gcc dot gnu.org
@ 2023-06-03  8:58 ` olegendo at gcc dot gnu.org
  2023-06-03  9:02 ` pinskia at gcc dot gnu.org
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: olegendo at gcc dot gnu.org @ 2023-06-03  8:58 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #13 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #12)
> Created attachment 55239 [details]
> Patch which does work on this
> 
> Though, I need to double to make sure it works for other cases still.
> sh is the case where we have a non-CC mode register for the flags which is
> why this was harder than other targets.

Thanks for looking at this (after 10 years .. wow, time flies).


> diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
> index 4622dba0121..ae1a0f7eb7c 100644
> --- a/gcc/config/sh/sh.md
> +++ b/gcc/config/sh/sh.md
> @@ -1510,7 +1510,7 @@ (define_expand "movsicc"
>    rtx op0 = XEXP (operands[1], 0);
>    rtx op1 = XEXP (operands[1], 1);
>
> -  if (! currently_expanding_to_rtl)
> +  if (0 && ! currently_expanding_to_rtl)
>      FAIL;
>
>   switch (code)
> diff --git a/gcc/config/sh/t-sh b/gcc/config/sh/t-sh
> index 1cc8c39d2a8..8ffcc288cf3 100644
> --- a/gcc/config/sh/t-sh
> +++ b/gcc/config/sh/t-sh
> @@ -89,8 +89,8 @@ MULTILIB_OSDIRNAMES = \
>  	m4a=!m4a $(OTHER_ENDIAN)/m4a=!$(OTHER_ENDIAN)/m4a \
>  	m4al=!m4al $(OTHER_ENDIAN)/m4al=!$(OTHER_ENDIAN)/m4al
>  
> -$(out_object_file): gt-sh.h
> -gt-sh.h : s-gtype ; @true
> +#$(out_object_file): gt-sh.h
> +#gt-sh.h : s-gtype ; @true
>  
>  # Local Variables:
>  # mode: Makefile

Are these hunks intentional?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (10 preceding siblings ...)
  2023-06-03  8:58 ` olegendo at gcc dot gnu.org
@ 2023-06-03  9:02 ` pinskia at gcc dot gnu.org
  2023-06-03 21:50 ` pinskia at gcc dot gnu.org
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-03  9:02 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #14 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #13)
> (In reply to Andrew Pinski from comment #12)
> > Created attachment 55239 [details]
> > Patch which does work on this
> > 
> > Though, I need to double to make sure it works for other cases still.
> > sh is the case where we have a non-CC mode register for the flags which is
> > why this was harder than other targets.
> 
> Thanks for looking at this (after 10 years .. wow, time flies).
> 
> 
> > diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
> > index 4622dba0121..ae1a0f7eb7c 100644
> > --- a/gcc/config/sh/sh.md
> > +++ b/gcc/config/sh/sh.md
> > @@ -1510,7 +1510,7 @@ (define_expand "movsicc"
> >    rtx op0 = XEXP (operands[1], 0);
> >    rtx op1 = XEXP (operands[1], 1);
> >
> > -  if (! currently_expanding_to_rtl)
> > +  if (0 && ! currently_expanding_to_rtl)
> >      FAIL;
> >
> >   switch (code)
> > diff --git a/gcc/config/sh/t-sh b/gcc/config/sh/t-sh
> > index 1cc8c39d2a8..8ffcc288cf3 100644
> > --- a/gcc/config/sh/t-sh
> > +++ b/gcc/config/sh/t-sh
> > @@ -89,8 +89,8 @@ MULTILIB_OSDIRNAMES = \
> >  	m4a=!m4a $(OTHER_ENDIAN)/m4a=!$(OTHER_ENDIAN)/m4a \
> >  	m4al=!m4al $(OTHER_ENDIAN)/m4al=!$(OTHER_ENDIAN)/m4al
> >  
> > -$(out_object_file): gt-sh.h
> > -gt-sh.h : s-gtype ; @true
> > +#$(out_object_file): gt-sh.h
> > +#gt-sh.h : s-gtype ; @true
> >  
> >  # Local Variables:
> >  # mode: Makefile
> 
> Are these hunks intentional?

No. The second one was me trying to figure out why if I did make clean, the
build would then fail, I thought it wad related to that but it was a m2 (the
language) vs having a multilib dir named m2.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (11 preceding siblings ...)
  2023-06-03  9:02 ` pinskia at gcc dot gnu.org
@ 2023-06-03 21:50 ` pinskia at gcc dot gnu.org
  2023-06-03 22:18 ` pinskia at gcc dot gnu.org
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-03 21:50 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #15 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I was thinking of improving this is modify noce_get_condition here (we still
need the previous patch just in case we still get the T register).

Right now noce_get_condition will stop at:
(eq (reg:SI 147 t) (const_int 0 [0]))
Because of:
  /* If the condition variable is a register and is MODE_INT, accept it.  */
  cond = XEXP (SET_SRC (set), 0);
  tmp = XEXP (cond, 0);
  if (REG_P (tmp) && GET_MODE_CLASS (GET_MODE (tmp)) == MODE_INT
      && (GET_MODE (tmp) != BImode
          || !targetm.small_register_classes_for_mode_p (BImode)))

But since 147 is a hard register but maybe since it is hard register and
REGNO_REG_CLASS (147) == T_REGS and the register class of T_REGS is only one
register so consider a small register class.

This I think is related to the option 2 in comment #3.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (12 preceding siblings ...)
  2023-06-03 21:50 ` pinskia at gcc dot gnu.org
@ 2023-06-03 22:18 ` pinskia at gcc dot gnu.org
  2023-06-03 22:23 ` olegendo at gcc dot gnu.org
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-03 22:18 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #16 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Now I am curious if T_REG should be BImode rather than SImode ... Then ifcvt.cc
would not have to be modified. I Know BImode is newer than when sh target was
added but maybe if someone cares about the sh target could try to modify the
backend to use BImode for T_REG here.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (13 preceding siblings ...)
  2023-06-03 22:18 ` pinskia at gcc dot gnu.org
@ 2023-06-03 22:23 ` olegendo at gcc dot gnu.org
  2023-06-03 22:59 ` pinskia at gcc dot gnu.org
  2023-06-04  0:48 ` pinskia at gcc dot gnu.org
  16 siblings, 0 replies; 18+ messages in thread
From: olegendo at gcc dot gnu.org @ 2023-06-03 22:23 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #17 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #16)
> Now I am curious if T_REG should be BImode rather than SImode ... Then
> ifcvt.cc would not have to be modified. I Know BImode is newer than when sh
> target was added but maybe if someone cares about the sh target could try to
> modify the backend to use BImode for T_REG here.

Yeah, I know. Back then I was entertaining the idea.  But T-bit being SImode is
so intertwined with all other things in the backend, it looks like a big job to
undo/redo that.  For example there are patterns that use the T bit in other
arithmetic insns patterns.  Would need to check what would happen to those. 
Sounds like a can of worms.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (14 preceding siblings ...)
  2023-06-03 22:23 ` olegendo at gcc dot gnu.org
@ 2023-06-03 22:59 ` pinskia at gcc dot gnu.org
  2023-06-04  0:48 ` pinskia at gcc dot gnu.org
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-03 22:59 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #18 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55250
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55250&action=edit
This patch also fixes the issue by looking back

Here is a patch which does the lookback if the reg is a hard register and it is
considered a small reg class.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Bug rtl-optimization/58517] ifcvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use
  2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
                   ` (15 preceding siblings ...)
  2023-06-03 22:59 ` pinskia at gcc dot gnu.org
@ 2023-06-04  0:48 ` pinskia at gcc dot gnu.org
  16 siblings, 0 replies; 18+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-06-04  0:48 UTC (permalink / raw)
  To: gcc-bugs

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

--- Comment #19 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #18)
> Created attachment 55250 [details]
> This patch also fixes the issue by looking back
> 
> Here is a patch which does the lookback if the reg is a hard register and it
> is considered a small reg class.

Note with this version of the patch, you need to disable check against
currently_expanding_to_rtl in movsicc too. Otherwise you won't get the ifcvt
happening in general.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-06-04  0:48 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-23 21:50 [Bug target/58517] New: [SH] wrong code with subc and movsicc (-mpretend-cmove) after ce2 olegendo at gcc dot gnu.org
2013-10-05 20:22 ` [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and cc use olegendo at gcc dot gnu.org
2013-10-05 21:16 ` [Bug rtl-optimization/58517] icvt (after combine) puts ccreg clobbering insn between ccset insn and ccreg use olegendo at gcc dot gnu.org
2013-10-05 22:17 ` olegendo at gcc dot gnu.org
2013-10-05 23:23 ` steven at gcc dot gnu.org
2015-09-11 11:12 ` [Bug rtl-optimization/58517] ifcvt " ktkachov at gcc dot gnu.org
2023-06-02  1:38 ` pinskia at gcc dot gnu.org
2023-06-02  1:43 ` pinskia at gcc dot gnu.org
2023-06-02  1:44 ` pinskia at gcc dot gnu.org
2023-06-02  7:28 ` pinskia at gcc dot gnu.org
2023-06-02  7:57 ` pinskia at gcc dot gnu.org
2023-06-03  8:58 ` olegendo at gcc dot gnu.org
2023-06-03  9:02 ` pinskia at gcc dot gnu.org
2023-06-03 21:50 ` pinskia at gcc dot gnu.org
2023-06-03 22:18 ` pinskia at gcc dot gnu.org
2023-06-03 22:23 ` olegendo at gcc dot gnu.org
2023-06-03 22:59 ` pinskia at gcc dot gnu.org
2023-06-04  0:48 ` pinskia at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).