Bug 59291 - [SH] Group T bit related insns before combine
Summary: [SH] Group T bit related insns before combine
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.9.0
: P3 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-25 17:42 UTC by Oleg Endo
Modified: 2024-05-27 20:50 UTC (History)
1 user (show)

See Also:
Host:
Target: sh*-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2024-05-27 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Endo 2013-11-25 17:42:03 UTC
The following example function

struct result
{
  int a, b;
};

result test2 (int a, int b, int d)
{
  result r;
  r.a = a == b;
  r.b = d + b + 1;
  return r;
};


compiled with -O2 -m4 -ml results in

        cmp/eq  r5,r4
        add     r5,r6
        mov     r6,r1
        movt    r0
        rts
        add     #1,r1      <<< missed addc combine opportunity.


Initially the function is expanded to...
(notice the eq:SI that sets the T_REG and the movt that stores the comparison result are next to each other)

(note 1 0 6 NOTE_INSN_DELETED)
(note 6 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(insn 2 6 3 2 (set (reg/v:SI 166 [ a ])
        (reg:SI 4 r4 [ a ])) sh_tmp.cpp:7 -1
     (nil))
(insn 3 2 4 2 (set (reg/v:SI 167 [ b ])
        (reg:SI 5 r5 [ b ])) sh_tmp.cpp:7 -1
     (nil))
(insn 4 3 5 2 (set (reg/v:SI 168 [ d ])
        (reg:SI 6 r6 [ d ])) sh_tmp.cpp:7 -1
     (nil))
(note 5 4 8 2 NOTE_INSN_FUNCTION_BEG)
(insn 8 5 9 2 (set (reg:SI 147 t)
        (eq:SI (reg/v:SI 166 [ a ])
            (reg/v:SI 167 [ b ]))) sh_tmp.cpp:9 -1
     (nil))
(insn 9 8 10 2 (set (reg:SI 170 [ D.1945 ])
        (reg:SI 147 t)) sh_tmp.cpp:9 -1
     (nil))
(insn 10 9 11 2 (set (subreg:SI (reg:DI 164 [ D.1936 ]) 0)
        (reg:SI 170 [ D.1945 ])) sh_tmp.cpp:11 -1
     (nil))
(insn 11 10 12 2 (set (reg:SI 171 [ D.1946 ])
        (plus:SI (reg/v:SI 168 [ d ])
            (reg/v:SI 167 [ b ]))) sh_tmp.cpp:10 -1
     (nil))
(insn 12 11 13 2 (set (reg:SI 172 [ D.1946 ])
        (plus:SI (reg:SI 171 [ D.1946 ])
            (const_int 1 [0x1]))) sh_tmp.cpp:10 -1
     (nil))
(insn 13 12 14 2 (set (subreg:SI (reg:DI 164 [ D.1936 ]) 4)
        (reg:SI 172 [ D.1946 ])) sh_tmp.cpp:11 -1
     (nil))
(insn 14 13 18 2 (set (reg:DI 165 [ <retval> ])
        (reg:DI 164 [ D.1936 ])) sh_tmp.cpp:11 -1
     (nil))
(insn 18 14 21 2 (set (reg/i:DI 0 r0)
        (reg:DI 165 [ <retval> ])) sh_tmp.cpp:12 -1
     (nil))
(insn 21 18 0 2 (use (reg/i:DI 0 r0)) sh_tmp.cpp:12 -1
     (nil))


The fwprop1 pass however, changes this to...

;;    total ref usage 48{26d,22u,0e} in 9{9 regular + 0 call} insns.
(note 6 0 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)
(insn 2 6 3 2 (set (reg/v:SI 166 [ a ])
        (reg:SI 4 r4 [ a ])) sh_tmp.cpp:7 257 {movsi_ie}
     (expr_list:REG_DEAD (reg:SI 4 r4 [ a ])
        (nil)))
(insn 3 2 4 2 (set (reg/v:SI 167 [ b ])
        (reg:SI 5 r5 [ b ])) sh_tmp.cpp:7 257 {movsi_ie}
     (expr_list:REG_DEAD (reg:SI 5 r5 [ b ])
        (nil)))
(insn 4 3 5 2 (set (reg/v:SI 168 [ d ])
        (reg:SI 6 r6 [ d ])) sh_tmp.cpp:7 257 {movsi_ie}
     (expr_list:REG_DEAD (reg:SI 6 r6 [ d ])
        (nil)))
(note 5 4 8 2 NOTE_INSN_FUNCTION_BEG)
(insn 8 5 11 2 (set (reg:SI 147 t)
        (eq:SI (reg/v:SI 166 [ a ])
            (reg/v:SI 167 [ b ]))) sh_tmp.cpp:9 17 {cmpeqsi_t}
     (expr_list:REG_DEAD (reg/v:SI 166 [ a ])
        (nil)))
(insn 11 8 12 2 (set (reg:SI 171 [ D.1946 ])
        (plus:SI (reg/v:SI 168 [ d ])
            (reg/v:SI 167 [ b ]))) sh_tmp.cpp:10 75 {*addsi3_compact}
     (expr_list:REG_DEAD (reg/v:SI 168 [ d ])
        (expr_list:REG_DEAD (reg/v:SI 167 [ b ])
            (nil))))
(insn 12 11 26 2 (set (reg:SI 172 [ D.1946 ])
        (plus:SI (reg:SI 171 [ D.1946 ])
            (const_int 1 [0x1]))) sh_tmp.cpp:10 75 {*addsi3_compact}
     (expr_list:REG_DEAD (reg:SI 171 [ D.1946 ])
        (nil)))
(insn 26 12 27 2 (set (reg:SI 0 r0)
        (reg:SI 147 t)) sh_tmp.cpp:12 399 {movt}
     (expr_list:REG_DEAD (reg:SI 147 t)
        (nil)))
(insn 27 26 21 2 (set (reg:SI 1 r1 [+4 ])
        (reg:SI 172 [ D.1946 ])) sh_tmp.cpp:12 257 {movsi_ie}
     (expr_list:REG_DEAD (reg:SI 172 [ D.1946 ])
        (nil)))
(insn 21 27 0 2 (use (reg/i:DI 0 r0)) sh_tmp.cpp:12 -1
     (nil))

... which makes the T_REG live across the addsi insns and thus combine will fail to replace them with an addc insn which clobbers the T_REG.
Insns that use the T_REG (the movt insn in this case) should be reordered in such a way that they immediately follow the insn that sets the T_REG (the cmpeqsi insn in this case) before combine.  This would increase the probability of combine successes.
However, there could be some unlucky cases.  If the movt destination is 'r0' as above, and it is moved above other insns which might clobber/need the r0 reg (e.g. tst #imm, logical #imm ops etc), there could be reload problems or the code might get worse for such sequences.
Comment 1 pietro 2024-05-27 19:49:02 UTC
Is this still valid? GCC 14 on the Compiler Explorer[0] show GCC 9.5 producing the same assembly, but 12 and above (it doesn't have SH GCC 10 and 11) produces:

_test2:
 cmp/eq	r5,r4
 mov	r5,r1
 add	r6,r1
 add	#1,r1
 rts
 movt	r0

[0]: https://godbolt.org/z/668ax5ehj
Comment 2 Oleg Endo 2024-05-27 20:50:29 UTC
(In reply to pietro from comment #1)
> Is this still valid? GCC 14 on the Compiler Explorer[0] show GCC 9.5
> producing the same assembly, but 12 and above (it doesn't have SH GCC 10 and
> 11) produces:
> 
> _test2:
>  cmp/eq	r5,r4
>  mov	r5,r1
>  add	r6,r1
>  add	#1,r1
>  rts
>  movt	r0
> 
> [0]: https://godbolt.org/z/668ax5ehj

Yes, still valid.  Nothing has been done to explicitly address or improve the issue.  The ideal code should be something like:

        cmp/eq  r5,r4
        movt    r0
        sett
        addc    r5,r6
        rts
        mov     r6,r1