Message ID | 8a65498338913b3902abaf7ac325df4ce230cd08.1518389741.git.segher@kernel.crashing.org |
---|---|
State | New |
Headers | show |
Series | combine: Update links correctly for new I2 (PR84169) | expand |
On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote: > If there is a LOG_LINK between two insns, this means those two insns > can be combined, as far as dataflow is concerned. There never should > be a LOG_LINK between two unrelated insns. If there is one, combine > will try to combine the insns without doing all the needed checks if > the earlier destination is used before the later insn, etc. > > Unfortunately we do not update the LOG_LINKs correctly in some cases. > This patch fixes at least some of those cases. > > This fixes the PR's testcase on aarch64. Also tested on 30+ cross > compiler, and on powerpc64-linux {-m32,-m64}. Will test on x86_64 > as well before committing. Will you check in the testcase too? My preference would be something like following, so that it can be torture-tested on all targets. --- gcc/testsuite/gcc.c-torture/execute/pr84169.c 2018-01-12 10:39:42.940283691 +0100 +++ gcc/testsuite/gcc.c-torture/execute/pr84169.c 2018-02-12 17:11:18.970878978 +0100 @@ -0,0 +1,25 @@ +/* PR rtl-optimization/84169 */ + +#ifdef __SIZEOF_INT128__ +typedef unsigned __int128 T; +#else +typedef unsigned long long T; +#endif + +T b; + +static __attribute__ ((noipa)) T +foo (T c, T d, T e, T f, T g, T h) +{ + __builtin_mul_overflow ((unsigned char) h, -16, &h); + return b + h; +} + +int +main () +{ + T x = foo (0, 0, 0, 0, 0, 4); + if (x != -64) + __builtin_abort (); + return 0; +} Jakub
On Mon, Feb 12, 2018 at 05:12:20PM +0100, Jakub Jelinek wrote: > On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote: > > If there is a LOG_LINK between two insns, this means those two insns > > can be combined, as far as dataflow is concerned. There never should > > be a LOG_LINK between two unrelated insns. If there is one, combine > > will try to combine the insns without doing all the needed checks if > > the earlier destination is used before the later insn, etc. > > > > Unfortunately we do not update the LOG_LINKs correctly in some cases. > > This patch fixes at least some of those cases. > > > > This fixes the PR's testcase on aarch64. Also tested on 30+ cross > > compiler, and on powerpc64-linux {-m32,-m64}. Will test on x86_64 > > as well before committing. > > Will you check in the testcase too? Yes, but it is dg-do run and I don't have a native aarch compiler built for it yet, so that will be a later patch. > My preference would be something like following, so that it can > be torture-tested on all targets. Thanks! Segher
On Mon, Feb 12, 2018 at 05:12:20PM +0100, Jakub Jelinek wrote: > On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote: > > If there is a LOG_LINK between two insns, this means those two insns > > can be combined, as far as dataflow is concerned. There never should > > be a LOG_LINK between two unrelated insns. If there is one, combine > > will try to combine the insns without doing all the needed checks if > > the earlier destination is used before the later insn, etc. > > > > Unfortunately we do not update the LOG_LINKs correctly in some cases. > > This patch fixes at least some of those cases. > > > > This fixes the PR's testcase on aarch64. Also tested on 30+ cross > > compiler, and on powerpc64-linux {-m32,-m64}. Will test on x86_64 > > as well before committing. > > Will you check in the testcase too? > > My preference would be something like following, so that it can > be torture-tested on all targets. Works flawlessly everywhere. I've committed this now, thanks again! Segher
On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote: > 2018-02-12 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/84169 > * combine.c (try_combine): New variable split_i2i3. Set it to true if > we generated a parallel as new i3 and we split that to new i2 and i3 > instructions. Handle split_i2i3 similar to swap_i2i3: scan the > LOG_LINKs of i3 to see which of those need to link to i2 now. Link > those to i2, not i1. Partially rewrite this scan code. > + unsigned int regno = REGNO (SET_DEST (x)); This line ICEs with rtl checking on both x86_64-linux and i686-linux on gcc.c-torture/compile/pr66168.c: +FAIL: gcc.c-torture/compile/pr66168.c -O2 (internal compiler error) +FAIL: gcc.c-torture/compile/pr66168.c -O2 (test for excess errors) +FAIL: gcc.c-torture/compile/pr66168.c -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error) +FAIL: gcc.c-torture/compile/pr66168.c -O2 -flto -fno-use-linker-plugin -flto-partition=none (test for excess errors) +FAIL: gcc.c-torture/compile/pr66168.c -O3 -g (internal compiler error) +FAIL: gcc.c-torture/compile/pr66168.c -O3 -g (test for excess errors) +FAIL: gcc.c-torture/compile/pr66168.c -Os (internal compiler error) +FAIL: gcc.c-torture/compile/pr66168.c -Os (test for excess errors) /home/jakub/src/gcc/gcc/testsuite/gcc.c-torture/compile/pr66168.c:15:1: internal compiler error: RTL check: expected code 'reg', have 'subreg' in rhs_regno, at rtl.h:1896 0x6755ff rtl_check_failed_code1(rtx_def const*, rtx_code, char const*, int, char const*) ../../gcc/rtl.c:844 0x7fcdd2 rhs_regno ../../gcc/rtl.h:1896 0x807e75 rhs_regno ../../gcc/rtl.h:1447 0x807e75 try_combine ../../gcc/combine.c:4286 0x1680cc1 combine_instructions ../../gcc/combine.c:1320 0x1680cc1 rest_of_handle_combine ../../gcc/combine.c:14881 0x1680cc1 execute ../../gcc/combine.c:14926 Jakub
diff --git a/gcc/combine.c b/gcc/combine.c index 870bc77..204368e 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -2737,6 +2737,7 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, /* Notes that I1, I2 or I3 is a MULT operation. */ int have_mult = 0; int swap_i2i3 = 0; + int split_i2i3 = 0; int changed_i3_dest = 0; int maxreg; @@ -4167,6 +4168,9 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, } insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes); + + if (insn_code_number >= 0) + split_i2i3 = 1; } } @@ -4334,44 +4338,45 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, if (swap_i2i3) { - rtx_insn *insn; - struct insn_link *link; - rtx ni2dest; - /* I3 now uses what used to be its destination and which is now I2's destination. This requires us to do a few adjustments. */ PATTERN (i3) = newpat; adjust_for_new_dest (i3); + } - /* We need a LOG_LINK from I3 to I2. But we used to have one, - so we still will. + if (swap_i2i3 || split_i2i3) + { + /* We might need a LOG_LINK from I3 to I2. But then we used to + have one, so we still will. However, some later insn might be using I2's dest and have - a LOG_LINK pointing at I3. We must remove this link. - The simplest way to remove the link is to point it at I1, - which we know will be a NOTE. */ + a LOG_LINK pointing at I3. We should change it to point at + I2 instead. */ /* newi2pat is usually a SET here; however, recog_for_combine might have added some clobbers. */ - if (GET_CODE (newi2pat) == PARALLEL) - ni2dest = SET_DEST (XVECEXP (newi2pat, 0, 0)); - else - ni2dest = SET_DEST (newi2pat); + rtx x = newi2pat; + if (GET_CODE (x) == PARALLEL) + x = XVECEXP (newi2pat, 0, 0); - for (insn = NEXT_INSN (i3); - insn && (this_basic_block->next_bb == EXIT_BLOCK_PTR_FOR_FN (cfun) - || insn != BB_HEAD (this_basic_block->next_bb)); + unsigned int regno = REGNO (SET_DEST (x)); + + bool done = false; + for (rtx_insn *insn = NEXT_INSN (i3); + !done + && insn + && NONDEBUG_INSN_P (insn) + && BLOCK_FOR_INSN (insn) == this_basic_block; insn = NEXT_INSN (insn)) { - if (NONDEBUG_INSN_P (insn) - && reg_referenced_p (ni2dest, PATTERN (insn))) - { - FOR_EACH_LOG_LINK (link, insn) - if (link->insn == i3) - link->insn = i1; - - break; - } + struct insn_link *link; + FOR_EACH_LOG_LINK (link, insn) + if (link->insn == i3 && link->regno == regno) + { + link->insn = i2; + done = true; + break; + } } }