| Message ID | 20220105093941.GR2646553@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | ifcvt: Check for asm goto at the end of then_bb/else_bb in ifcvt [PR103908] | expand |
On Wed, 5 Jan 2022, Jakub Jelinek wrote: > Hi! > > On the following testcase, RTL ifcvt sees then_bb > (note 7 6 8 3 [bb 3] NOTE_INSN_BASIC_BLOCK) > (insn 8 7 9 3 (set (mem/c:SI (symbol_ref:DI ("b") [flags 0x2] <var_decl 0x7fdccf5b0cf0 b>) [1 b+0 S4 A32]) > (const_int 1 [0x1])) "pr103908.c":6:7 81 {*movsi_internal} > (nil)) > (jump_insn 9 8 13 3 (parallel [ > (asm_operands/v ("# insn 1") ("") 0 [] > [] > [ > (label_ref:DI 21) > ] pr103908.c:7) > (clobber (reg:CC 17 flags)) > ]) "pr103908.c":7:5 -1 > (expr_list:REG_UNUSED (reg:CC 17 flags) > (nil)) > -> 21) > and similarly else_bb (just with a different asm_operands template). > It checks that those basic blocks have a single successor and > uses last_active_insn which intentionally skips over JUMP_INSNs, sees > both basic blocks contain the same set and merges them (or if the > sets are different, attempts some other noce optimization). > But we can't assume that the jump, even when it has only a single successor, > has no side-effects. > > The following patch fixes it by checking specially for asm goto, but > I wonder if it wouldn't be better to > if (JUMP_P (BB_END (test_bb)) && !onlyjump_p (BB_END (test_bb))) > return false; > and give up on any other hypothetical single successor jumps. > > The patch below has been bootstrapped/regtested on x86_64-linux and > i686-linux, I could bootstrap the !onlyjump_p version too... I'd prefer checking with onlyjump_p. OK with that change. Thanks, Richard. > 2022-01-05 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/103908 > * ifcvt.c (bb_valid_for_noce_process_p): Punt on bbs ending with > asm goto. > > * gcc.target/i386/pr103908.c: New test. > > --- gcc/ifcvt.c.jj 2022-01-03 10:40:42.919140216 +0100 > +++ gcc/ifcvt.c 2022-01-04 23:50:40.962529052 +0100 > @@ -3064,6 +3064,13 @@ bb_valid_for_noce_process_p (basic_block > > if (!insn_valid_noce_process_p (last_insn, cc)) > return false; > + > + /* Punt for blocks ending with asm goto, last_active_insn > + ignores JUMP_INSNs. */ > + if (JUMP_P (BB_END (test_bb)) > + && asm_noperands (PATTERN (BB_END (test_bb))) >= 0) > + return false; > + > last_set = single_set (last_insn); > > rtx x = SET_DEST (last_set); > --- gcc/testsuite/gcc.target/i386/pr103908.c.jj 2022-01-04 23:58:02.992341275 +0100 > +++ gcc/testsuite/gcc.target/i386/pr103908.c 2022-01-04 23:57:39.399671539 +0100 > @@ -0,0 +1,24 @@ > +/* PR rtl-optimization/103908 */ > +/* { dg-do compile } */ > +/* { dg-options "-O1 -fdisable-tree-cselim -fno-tree-sink" } */ > +/* { dg-final { scan-assembler "# insn 1" } } */ > +/* { dg-final { scan-assembler "# insn 2" } } */ > + > +int a, b; > + > +void > +foo (void) > +{ > + if (a) > + { > + b = 1; > + asm goto ("# insn 1" : : : : lab1); > + lab1:; > + } > + else > + { > + b = 1; > + asm goto ("# insn 2" : : : : lab2); > + lab2:; > + } > +} > > Jakub > >
--- gcc/ifcvt.c.jj 2022-01-03 10:40:42.919140216 +0100 +++ gcc/ifcvt.c 2022-01-04 23:50:40.962529052 +0100 @@ -3064,6 +3064,13 @@ bb_valid_for_noce_process_p (basic_block if (!insn_valid_noce_process_p (last_insn, cc)) return false; + + /* Punt for blocks ending with asm goto, last_active_insn + ignores JUMP_INSNs. */ + if (JUMP_P (BB_END (test_bb)) + && asm_noperands (PATTERN (BB_END (test_bb))) >= 0) + return false; + last_set = single_set (last_insn); rtx x = SET_DEST (last_set); --- gcc/testsuite/gcc.target/i386/pr103908.c.jj 2022-01-04 23:58:02.992341275 +0100 +++ gcc/testsuite/gcc.target/i386/pr103908.c 2022-01-04 23:57:39.399671539 +0100 @@ -0,0 +1,24 @@ +/* PR rtl-optimization/103908 */ +/* { dg-do compile } */ +/* { dg-options "-O1 -fdisable-tree-cselim -fno-tree-sink" } */ +/* { dg-final { scan-assembler "# insn 1" } } */ +/* { dg-final { scan-assembler "# insn 2" } } */ + +int a, b; + +void +foo (void) +{ + if (a) + { + b = 1; + asm goto ("# insn 1" : : : : lab1); + lab1:; + } + else + { + b = 1; + asm goto ("# insn 2" : : : : lab2); + lab2:; + } +}
Hi! On the following testcase, RTL ifcvt sees then_bb (note 7 6 8 3 [bb 3] NOTE_INSN_BASIC_BLOCK) (insn 8 7 9 3 (set (mem/c:SI (symbol_ref:DI ("b") [flags 0x2] <var_decl 0x7fdccf5b0cf0 b>) [1 b+0 S4 A32]) (const_int 1 [0x1])) "pr103908.c":6:7 81 {*movsi_internal} (nil)) (jump_insn 9 8 13 3 (parallel [ (asm_operands/v ("# insn 1") ("") 0 [] [] [ (label_ref:DI 21) ] pr103908.c:7) (clobber (reg:CC 17 flags)) ]) "pr103908.c":7:5 -1 (expr_list:REG_UNUSED (reg:CC 17 flags) (nil)) -> 21) and similarly else_bb (just with a different asm_operands template). It checks that those basic blocks have a single successor and uses last_active_insn which intentionally skips over JUMP_INSNs, sees both basic blocks contain the same set and merges them (or if the sets are different, attempts some other noce optimization). But we can't assume that the jump, even when it has only a single successor, has no side-effects. The following patch fixes it by checking specially for asm goto, but I wonder if it wouldn't be better to if (JUMP_P (BB_END (test_bb)) && !onlyjump_p (BB_END (test_bb))) return false; and give up on any other hypothetical single successor jumps. The patch below has been bootstrapped/regtested on x86_64-linux and i686-linux, I could bootstrap the !onlyjump_p version too... 2022-01-05 Jakub Jelinek <jakub@redhat.com> PR rtl-optimization/103908 * ifcvt.c (bb_valid_for_noce_process_p): Punt on bbs ending with asm goto. * gcc.target/i386/pr103908.c: New test. Jakub