| Message ID | 36e7b4de-3d53-88c8-7819-54a2dacca710@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote: > Hi, > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation > where SLSR will ICE when exposed to a cast from integer to pointer. This > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly > of pointer type. This patch recognizes when pointer arithmetic is taking > place and ensures that we use a POINTER_PLUS_EXPR at all such times. In > the case of the PR, this occurs in the logic where the stride S is a known > constant value, but the same problem could occur when it is an SSA_NAME > without known value. Both possibilities are handled here. > > Fixing the code to ensure that the unknown stride case always uses an > initializer for a negative increment allows us to remove the stopgap fix > added for PR77937 as well. > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > regressions, committed. Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on X86_64 before committing these patches, because you broke it for the third time in the last couple of days. markus@x4 ffmpeg % cat h264dsp.i extern int fn2(int); extern int fn3(int); int a, b, c; void fn1(long p1) { char *d; for (;; d += p1) { d[0] = fn2(1 >> c >> 1); fn2(c >> a); d[1] = fn3(d[1]) >> 1; d[6] = fn3(d[6] * b + 1) >> 1; d[7] = fn3(d[7] * b + 1) >> 1; d[8] = fn3(d[8] * b + 1) >> 1; } } markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i h264dsp.i: In function ‘fn1’: h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at gimple-ssa-strength-reduction.c:3375 void fn1(long p1) { ^~~ 0x12773a9 replace_one_candidate ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375 0x127af77 replace_profitable_candidates ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486 0x127aeeb replace_profitable_candidates ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495 0x127f3ee analyze_candidates_and_replace ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574 0x127f3ee execute ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648
On 2016.10.18 at 05:13 +0200, Markus Trippelsdorf wrote: > On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote: > > Hi, > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation > > where SLSR will ICE when exposed to a cast from integer to pointer. This > > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a > > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly > > of pointer type. This patch recognizes when pointer arithmetic is taking > > place and ensures that we use a POINTER_PLUS_EXPR at all such times. In > > the case of the PR, this occurs in the logic where the stride S is a known > > constant value, but the same problem could occur when it is an SSA_NAME > > without known value. Both possibilities are handled here. > > > > Fixing the code to ensure that the unknown stride case always uses an > > initializer for a negative increment allows us to remove the stopgap fix > > added for PR77937 as well. > > > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > > regressions, committed. > > Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on > X86_64 before committing these patches, because you broke it for the > third time in the last couple of days. > > markus@x4 ffmpeg % cat h264dsp.i > extern int fn2(int); > extern int fn3(int); > int a, b, c; > void fn1(long p1) { > char *d; > for (;; d += p1) { > d[0] = fn2(1 >> c >> 1); > fn2(c >> a); > d[1] = fn3(d[1]) >> 1; > d[6] = fn3(d[6] * b + 1) >> 1; > d[7] = fn3(d[7] * b + 1) >> 1; > d[8] = fn3(d[8] * b + 1) >> 1; > } > } > > markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i > h264dsp.i: In function ‘fn1’: > h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at gimple-ssa-strength-reduction.c:3375 > void fn1(long p1) { > ^~~ > 0x12773a9 replace_one_candidate > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375 > 0x127af77 replace_profitable_candidates > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486 > 0x127aeeb replace_profitable_candidates > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495 > 0x127f3ee analyze_candidates_and_replace > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574 > 0x127f3ee execute > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648 Just figured out that this testcase is identical to: gcc/testsuite/gcc.dg/torture/pr77937-2.c So please run the testsuite on X86_64 in the future.
On 18 October 2016 at 05:18, Markus Trippelsdorf <markus@trippelsdorf.de> wrote: > On 2016.10.18 at 05:13 +0200, Markus Trippelsdorf wrote: >> On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote: >> > Hi, >> > >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation >> > where SLSR will ICE when exposed to a cast from integer to pointer. This >> > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a >> > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly >> > of pointer type. This patch recognizes when pointer arithmetic is taking >> > place and ensures that we use a POINTER_PLUS_EXPR at all such times. In >> > the case of the PR, this occurs in the logic where the stride S is a known >> > constant value, but the same problem could occur when it is an SSA_NAME >> > without known value. Both possibilities are handled here. >> > >> > Fixing the code to ensure that the unknown stride case always uses an >> > initializer for a negative increment allows us to remove the stopgap fix >> > added for PR77937 as well. >> > >> > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no >> > regressions, committed. >> >> Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on >> X86_64 before committing these patches, because you broke it for the >> third time in the last couple of days. >> >> markus@x4 ffmpeg % cat h264dsp.i >> extern int fn2(int); >> extern int fn3(int); >> int a, b, c; >> void fn1(long p1) { >> char *d; >> for (;; d += p1) { >> d[0] = fn2(1 >> c >> 1); >> fn2(c >> a); >> d[1] = fn3(d[1]) >> 1; >> d[6] = fn3(d[6] * b + 1) >> 1; >> d[7] = fn3(d[7] * b + 1) >> 1; >> d[8] = fn3(d[8] * b + 1) >> 1; >> } >> } >> >> markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i >> h264dsp.i: In function ‘fn1’: >> h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at gimple-ssa-strength-reduction.c:3375 >> void fn1(long p1) { >> ^~~ >> 0x12773a9 replace_one_candidate >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375 >> 0x127af77 replace_profitable_candidates >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486 >> 0x127aeeb replace_profitable_candidates >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495 >> 0x127f3ee analyze_candidates_and_replace >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574 >> 0x127f3ee execute >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648 > > Just figured out that this testcase is identical to: > gcc/testsuite/gcc.dg/torture/pr77937-2.c > > So please run the testsuite on X86_64 in the future. > I'm not sure whether Markus means that pr77937-2 fails since this commit? I'm seeing ICEs on pr77937-2 on some arm targets: http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241285/report-build-info.html but I'm not 100% sure it was caused by this patch (the regression happened between r241273 and r241285), I haven't looked in details yet. Christophe > -- > Markus
On 2016.10.18 at 11:19 +0200, Christophe Lyon wrote: > On 18 October 2016 at 05:18, Markus Trippelsdorf <markus@trippelsdorf.de> wrote: > > On 2016.10.18 at 05:13 +0200, Markus Trippelsdorf wrote: > >> On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote: > >> > Hi, > >> > > >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation > >> > where SLSR will ICE when exposed to a cast from integer to pointer. This > >> > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a > >> > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly > >> > of pointer type. This patch recognizes when pointer arithmetic is taking > >> > place and ensures that we use a POINTER_PLUS_EXPR at all such times. In > >> > the case of the PR, this occurs in the logic where the stride S is a known > >> > constant value, but the same problem could occur when it is an SSA_NAME > >> > without known value. Both possibilities are handled here. > >> > > >> > Fixing the code to ensure that the unknown stride case always uses an > >> > initializer for a negative increment allows us to remove the stopgap fix > >> > added for PR77937 as well. > >> > > >> > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > >> > regressions, committed. > >> > >> Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on > >> X86_64 before committing these patches, because you broke it for the > >> third time in the last couple of days. > >> > >> markus@x4 ffmpeg % cat h264dsp.i > >> extern int fn2(int); > >> extern int fn3(int); > >> int a, b, c; > >> void fn1(long p1) { > >> char *d; > >> for (;; d += p1) { > >> d[0] = fn2(1 >> c >> 1); > >> fn2(c >> a); > >> d[1] = fn3(d[1]) >> 1; > >> d[6] = fn3(d[6] * b + 1) >> 1; > >> d[7] = fn3(d[7] * b + 1) >> 1; > >> d[8] = fn3(d[8] * b + 1) >> 1; > >> } > >> } > >> > >> markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i > >> h264dsp.i: In function ‘fn1’: > >> h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at gimple-ssa-strength-reduction.c:3375 > >> void fn1(long p1) { > >> ^~~ > >> 0x12773a9 replace_one_candidate > >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375 > >> 0x127af77 replace_profitable_candidates > >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486 > >> 0x127aeeb replace_profitable_candidates > >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495 > >> 0x127f3ee analyze_candidates_and_replace > >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574 > >> 0x127f3ee execute > >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648 > > > > Just figured out that this testcase is identical to: > > gcc/testsuite/gcc.dg/torture/pr77937-2.c > > > > So please run the testsuite on X86_64 in the future. > > > > > I'm not sure whether Markus means that pr77937-2 fails since this > commit? > > I'm seeing ICEs on pr77937-2 on some arm targets: > http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241285/report-build-info.html > > but I'm not 100% sure it was caused by this patch (the regression > happened between r241273 and r241285), I haven't looked in details > yet. Yes, this is caused by this patch.
On Tue, 2016-10-18 at 05:13 +0200, Markus Trippelsdorf wrote: > On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote: > > Hi, > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation > > where SLSR will ICE when exposed to a cast from integer to pointer. This > > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a > > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly > > of pointer type. This patch recognizes when pointer arithmetic is taking > > place and ensures that we use a POINTER_PLUS_EXPR at all such times. In > > the case of the PR, this occurs in the logic where the stride S is a known > > constant value, but the same problem could occur when it is an SSA_NAME > > without known value. Both possibilities are handled here. > > > > Fixing the code to ensure that the unknown stride case always uses an > > initializer for a negative increment allows us to remove the stopgap fix > > added for PR77937 as well. > > > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > > regressions, committed. > > Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on > X86_64 before committing these patches, because you broke it for the > third time in the last couple of days. Sorry, sorry. I did intend to build another stage1 cross and try this, but it just slipped my mind. Looks like I'll need to put the stopgap fix back in. I'll do that shortly. Meantime, -fno-slsr is a workaround. If I have trouble building ffmpeg with a cross, I'll check with you on testing a future patch. Bill > > markus@x4 ffmpeg % cat h264dsp.i > extern int fn2(int); > extern int fn3(int); > int a, b, c; > void fn1(long p1) { > char *d; > for (;; d += p1) { > d[0] = fn2(1 >> c >> 1); > fn2(c >> a); > d[1] = fn3(d[1]) >> 1; > d[6] = fn3(d[6] * b + 1) >> 1; > d[7] = fn3(d[7] * b + 1) >> 1; > d[8] = fn3(d[8] * b + 1) >> 1; > } > } > > markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i > h264dsp.i: In function ‘fn1’: > h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at gimple-ssa-strength-reduction.c:3375 > void fn1(long p1) { > ^~~ > 0x12773a9 replace_one_candidate > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375 > 0x127af77 replace_profitable_candidates > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486 > 0x127aeeb replace_profitable_candidates > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495 > 0x127f3ee analyze_candidates_and_replace > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574 > 0x127f3ee execute > ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648 > > > >
On 2016.10.18 at 08:15 -0500, Bill Schmidt wrote: > On Tue, 2016-10-18 at 05:13 +0200, Markus Trippelsdorf wrote: > > On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote: > > > Hi, > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation > > > where SLSR will ICE when exposed to a cast from integer to pointer. This > > > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a > > > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly > > > of pointer type. This patch recognizes when pointer arithmetic is taking > > > place and ensures that we use a POINTER_PLUS_EXPR at all such times. In > > > the case of the PR, this occurs in the logic where the stride S is a known > > > constant value, but the same problem could occur when it is an SSA_NAME > > > without known value. Both possibilities are handled here. > > > > > > Fixing the code to ensure that the unknown stride case always uses an > > > initializer for a negative increment allows us to remove the stopgap fix > > > added for PR77937 as well. > > > > > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > > > regressions, committed. > > > > Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on > > X86_64 before committing these patches, because you broke it for the > > third time in the last couple of days. > > Sorry, sorry. I did intend to build another stage1 cross and try this, > but it just slipped my mind. Looks like I'll need to put the stopgap > fix back in. I'll do that shortly. > > Meantime, -fno-slsr is a workaround. If I have trouble building ffmpeg > with a cross, I'll check with you on testing a future patch. I you wish I can send you a tarball with the preprocessed *.i files from ffmpeg, so that you can use a stage1 cross on them.
On Tue, 2016-10-18 at 15:30 +0200, Markus Trippelsdorf wrote: > I you wish I can send you a tarball with the preprocessed *.i files from > ffmpeg, so that you can use a stage1 cross on them. > That would be very helpful, thanks! Bill
Index: gcc/gimple-ssa-strength-reduction.c =================================================================== --- gcc/gimple-ssa-strength-reduction.c (revision 241245) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -2154,35 +2154,41 @@ create_add_on_incoming_edge (slsr_cand_t c, tree b basis_type = TREE_TYPE (basis_name); lhs = make_temp_ssa_name (basis_type, NULL, "slsr"); + /* Occasionally people convert integers to pointers without a + cast, leading us into trouble if we aren't careful. */ + enum tree_code plus_code + = POINTER_TYPE_P (basis_type) ? POINTER_PLUS_EXPR : PLUS_EXPR; + if (known_stride) { tree bump_tree; - enum tree_code code = PLUS_EXPR; + enum tree_code code = plus_code; widest_int bump = increment * wi::to_widest (c->stride); - if (wi::neg_p (bump)) + if (wi::neg_p (bump) && !POINTER_TYPE_P (basis_type)) { code = MINUS_EXPR; bump = -bump; } - bump_tree = wide_int_to_tree (basis_type, bump); + tree stride_type = POINTER_TYPE_P (basis_type) ? sizetype : basis_type; + bump_tree = wide_int_to_tree (stride_type, bump); new_stmt = gimple_build_assign (lhs, code, basis_name, bump_tree); } else { int i; - bool negate_incr = (!address_arithmetic_p && wi::neg_p (increment)); + bool negate_incr = !POINTER_TYPE_P (basis_type) && wi::neg_p (increment); i = incr_vec_index (negate_incr ? -increment : increment); gcc_assert (i >= 0); if (incr_vec[i].initializer) { - enum tree_code code = negate_incr ? MINUS_EXPR : PLUS_EXPR; + enum tree_code code = negate_incr ? MINUS_EXPR : plus_code; new_stmt = gimple_build_assign (lhs, code, basis_name, incr_vec[i].initializer); } else if (increment == 1) - new_stmt = gimple_build_assign (lhs, PLUS_EXPR, basis_name, c->stride); + new_stmt = gimple_build_assign (lhs, plus_code, basis_name, c->stride); else if (increment == -1) new_stmt = gimple_build_assign (lhs, MINUS_EXPR, basis_name, c->stride); @@ -2500,12 +2506,14 @@ record_increment (slsr_cand_t c, widest_int increm /* Optimistically record the first occurrence of this increment as providing an initializer (if it does); we will revise this opinion later if it doesn't dominate all other occurrences. - Exception: increments of -1, 0, 1 never need initializers; - and phi adjustments don't ever provide initializers. */ + Exception: increments of 0, 1 never need initializers; + and phi adjustments don't ever provide initializers. Note + that we only will see an increment of -1 here for pointer + arithmetic (otherwise we will have an initializer). */ if (c->kind == CAND_ADD && !is_phi_adjust && c->index == increment - && (increment > 1 || increment < -1) + && (increment > 1 || increment < 0) && (gimple_assign_rhs_code (c->cand_stmt) == PLUS_EXPR || gimple_assign_rhs_code (c->cand_stmt) == POINTER_PLUS_EXPR)) { @@ -2819,11 +2827,6 @@ analyze_increments (slsr_cand_t first_dep, machine && !POINTER_TYPE_P (first_dep->cand_type))) incr_vec[i].cost = COST_NEUTRAL; - /* FIXME: We don't handle pointers with a -1 increment yet. - They are usually unprofitable anyway. */ - else if (incr == -1 && POINTER_TYPE_P (first_dep->cand_type)) - incr_vec[i].cost = COST_INFINITE; - /* FORNOW: If we need to add an initializer, give up if a cast from the candidate's type to its stride's type can lose precision. This could eventually be handled better by expressly retaining the Index: gcc/testsuite/gcc.dg/torture/pr77916.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr77916.c (revision 0) +++ gcc/testsuite/gcc.dg/torture/pr77916.c (working copy) @@ -0,0 +1,20 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -Wno-int-conversion" } */ + +/* PR77916: This failed with "error: invalid (pointer) operands to plus/minus" + after SLSR. */ + +typedef struct +{ + void *f1; +} S; + +S *a; +int b; + +void +fn1 (void) +{ + for (; b; b++, a++) + a->f1 = b; +}