[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 --- Comment #10 from Ilya Enkovich --- Author: ienkovich Date: Mon Feb 29 14:32:24 2016 New Revision: 233811 URL: https://gcc.gnu.org/viewcvs?rev=233811&root=gcc&view=rev Log: gcc/testsuite/ 2016-02-29 Yuri Rumyantsev PR tree-optimization/69652 * gcc.dg/torture/pr69652.c: Delete test. * gcc.dg/vect/pr69652.c: New test. Added: trunk/gcc/testsuite/gcc.dg/vect/pr69652.c Removed: trunk/gcc/testsuite/gcc.dg/torture/pr69652.c Modified: trunk/gcc/testsuite/ChangeLog
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 Jakub Jelinek changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2016-02-03 CC||jakub at gcc dot gnu.org Target Milestone|--- |6.0 Ever confirmed|0 |1 --- Comment #1 from Jakub Jelinek --- Started with r233068
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 --- Comment #2 from Yuri Rumyantsev --- This is my fault - forgot to fix vuse for scalar statements which are crossed by masked stores during code motion. Fix is testing and will be sent for review tomorrow.
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 Jakub Jelinek changed: What|Removed |Added CC|jakub at redhat dot com| --- Comment #3 from Jakub Jelinek --- Clearly a bug in optimize_mask_stores. At the start of that function we have: ... mask__46.14_129 = vect__14.9_121 != vect__21.12_127; _46 = _14 != _21; mask__ifc__47.15_130 = mask__46.14_129; _ifc__47 = _46; MASK_STORE (vectp.16_132, 8B, mask__ifc__47.15_130, vect__22.13_128); vect__24.20_140 = MEM[(double *)vectp.18_138]; _24 = *_13; vect__25.21_141 = vect__21.12_127 + vect__24.20_140; _25 = _21 + _24; MASK_STORE (vectp.22_145, 8B, mask__ifc__47.15_130, vect__25.21_141); k_27 = k_28 + 1; ... Now, the MASK_STORE calls are processed from last to first, which is fine, we first move the second MASK_STORE and the vector stmts that feed it: vect__24.20_140 = MEM[(double *)vectp.18_138]; vect__25.21_141 = vect__21.12_127 + vect__24.20_140; MASK_STORE (vectp.22_145, 8B, mask__ifc__47.15_130, vect__25.21_141); but then continue trying to move the second MASK_STORE into the same conditional block, because it has the same mask. In this case it is wrong, because there is the scalar load in between (_24 = *_13) that just waits for DCE, but generally there could be arbitrary code. /* Put other masked stores with the same mask to STORE_BB. */ if (worklist.is_empty () || gimple_call_arg (worklist.last (), 2) != mask || worklist.last () != stmt1) break; has a simplistic check (doesn't consider other MASK_STORE unless the walking walked up to that stmt), but of course it doesn't work too well if some scalar stmts were skipped. I see various issues in that function: 1) wrong formatting: gsi_to = gsi_start_bb (store_bb); if (dump_enabled_p ()) { dump_printf_loc (MSG_NOTE, vect_location, "Move stmt to created bb\n"); dump_gimple_stmt (MSG_NOTE, TDF_SLIM, last, 0); } /* Move all stored value producers if possible. */ while (!gsi_end_p (gsi)) { The Move all stored value and everything below up to corresponding closing } should be moved two columns to the left 2) IMHO stmt1 should be set to NULL before that while (!gsi_end_p (gsi)), as the function is prepared to handle multiple bbs 3) next to gimple_vdef non-NULL break IMHO should be also gimple_has_volatile_ops -> break check, just for safety, we don't wanto to mishandle say volatile reads etc. 4) you have to skip over debug stmts if there are any, otherwise we have a -fcompare-debug issue 5) IMHO you should give up also for !is_gimple_assign, say trying to move an elemental function call into the conditional is just wrong 6) the /* Skip scalar statements. */ if (!VECTOR_TYPE_P (TREE_TYPE (lhs))) continue; should be reconsidered. IMHO if you have scalar stmts that feed just the stmts in the STORE_BB, there is no reason not to move them too, if you have scalar stmts that feed other stmts too, IMHO you should give up on them if they have a vuse; what code did you have in mind when adding the !VECTOR_TYPE_P check? 7) FOR_EACH_IMM_USE_FAST loop should ignore debug stmts, at least for decisions when to stop in some stmt; bet the debug stmts if there are any need to be reset if we move the def stmt that they are using, otherwise we risk -fcompare-debug issues 8) the worklist.last () != stmt1 check need to be -fcompare-debug friendly too, so if there are debug stmts in between the last moved stmt and the previous MASK_STORE, we need to handle it as if there aren't any debug stmts in between
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 --- Comment #4 from Yuri Rumyantsev --- Jacub, Thanks a lot for your detail comments! I've just sent a patch for review to gcc-patches. Could you please take a look on it? Best regards. Yuri. 2016-02-03 20:22 GMT+03:00 jakub at gcc dot gnu.org : > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 > > Jakub Jelinek changed: > >What|Removed |Added > > CC|jakub at redhat dot com| > > --- Comment #3 from Jakub Jelinek --- > Clearly a bug in optimize_mask_stores. > At the start of that function we have: > ... > mask__46.14_129 = vect__14.9_121 != vect__21.12_127; > _46 = _14 != _21; > mask__ifc__47.15_130 = mask__46.14_129; > _ifc__47 = _46; > MASK_STORE (vectp.16_132, 8B, mask__ifc__47.15_130, vect__22.13_128); > vect__24.20_140 = MEM[(double *)vectp.18_138]; > _24 = *_13; > vect__25.21_141 = vect__21.12_127 + vect__24.20_140; > _25 = _21 + _24; > MASK_STORE (vectp.22_145, 8B, mask__ifc__47.15_130, vect__25.21_141); > k_27 = k_28 + 1; > ... > Now, the MASK_STORE calls are processed from last to first, which is fine, we > first move the second MASK_STORE and the vector stmts that feed it: > vect__24.20_140 = MEM[(double *)vectp.18_138]; > vect__25.21_141 = vect__21.12_127 + vect__24.20_140; > MASK_STORE (vectp.22_145, 8B, mask__ifc__47.15_130, vect__25.21_141); > but then continue trying to move the second MASK_STORE into the same > conditional block, because it has the same mask. In this case it is wrong, > because there is > the scalar load in between (_24 = *_13) that just waits for DCE, but generally > there could be arbitrary code. > /* Put other masked stores with the same mask to STORE_BB. */ > if (worklist.is_empty () > || gimple_call_arg (worklist.last (), 2) != mask > || worklist.last () != stmt1) > break; > has a simplistic check (doesn't consider other MASK_STORE unless the walking > walked up to that stmt), but of course it doesn't work too well if some scalar > stmts were skipped. > > I see various issues in that function: > 1) wrong formatting: > gsi_to = gsi_start_bb (store_bb); > if (dump_enabled_p ()) > { > dump_printf_loc (MSG_NOTE, vect_location, >"Move stmt to created bb\n"); > dump_gimple_stmt (MSG_NOTE, TDF_SLIM, last, 0); > } > /* Move all stored value producers if possible. */ > while (!gsi_end_p (gsi)) > { > The Move all stored value and everything below up to corresponding closing } > should be moved two columns to the left > 2) IMHO stmt1 should be set to NULL before that while (!gsi_end_p (gsi)), > as the function is prepared to handle multiple bbs > 3) next to gimple_vdef non-NULL break IMHO should be also > gimple_has_volatile_ops -> break check, just for safety, we don't wanto to > mishandle say volatile reads etc. > 4) you have to skip over debug stmts if there are any, otherwise we have a > -fcompare-debug issue > 5) IMHO you should give up also for !is_gimple_assign, say trying to move an > elemental function call into the conditional is just wrong > 6) the > /* Skip scalar statements. */ > if (!VECTOR_TYPE_P (TREE_TYPE (lhs))) > continue; > should be reconsidered. IMHO if you have scalar stmts that feed just the > stmts > in the STORE_BB, there is no reason not to move them too, if you have scalar > stmts that feed other stmts too, IMHO you should give up on them if they have > a > vuse; what code did you have in mind when adding the !VECTOR_TYPE_P check? > 7) FOR_EACH_IMM_USE_FAST loop should ignore debug stmts, at least for > decisions > when to stop in some stmt; bet the debug stmts if there are any need to be > reset > if we move the def stmt that they are using, otherwise we risk -fcompare-debug > issues > 8) the worklist.last () != stmt1 check need to be -fcompare-debug friendly > too, > so if there are debug stmts in between the last moved stmt and the previous > MASK_STORE, we need to handle it as if there aren't any debug stmts in between > > -- > You are receiving this mail because: > You are on the CC list for the bug.
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 --- Comment #5 from Yuri Rumyantsev --- Jacub, I'd like to clarify one your remark: 5) IMHO you should give up also for !is_gimple_assign, say trying to move an elemental function call into the conditional is just wrong What's wrong in call motion? Note that masked stores and loads are also represented as call. I assume that likely simd clone function calls msut not be moved. Thanks ahead. Yuri. P.S. It means that my patch is not correct and should be fixed. 2016-02-04 17:48 GMT+03:00 Yuri Rumyantsev : > Jacub, > > Thanks a lot for your detail comments! > > I've just sent a patch for review to gcc-patches. Could you please > take a look on it? > > Best regards. > Yuri. > > 2016-02-03 20:22 GMT+03:00 jakub at gcc dot gnu.org > : >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 >> >> Jakub Jelinek changed: >> >>What|Removed |Added >> >> CC|jakub at redhat dot com| >> >> --- Comment #3 from Jakub Jelinek --- >> Clearly a bug in optimize_mask_stores. >> At the start of that function we have: >> ... >> mask__46.14_129 = vect__14.9_121 != vect__21.12_127; >> _46 = _14 != _21; >> mask__ifc__47.15_130 = mask__46.14_129; >> _ifc__47 = _46; >> MASK_STORE (vectp.16_132, 8B, mask__ifc__47.15_130, vect__22.13_128); >> vect__24.20_140 = MEM[(double *)vectp.18_138]; >> _24 = *_13; >> vect__25.21_141 = vect__21.12_127 + vect__24.20_140; >> _25 = _21 + _24; >> MASK_STORE (vectp.22_145, 8B, mask__ifc__47.15_130, vect__25.21_141); >> k_27 = k_28 + 1; >> ... >> Now, the MASK_STORE calls are processed from last to first, which is fine, we >> first move the second MASK_STORE and the vector stmts that feed it: >> vect__24.20_140 = MEM[(double *)vectp.18_138]; >> vect__25.21_141 = vect__21.12_127 + vect__24.20_140; >> MASK_STORE (vectp.22_145, 8B, mask__ifc__47.15_130, vect__25.21_141); >> but then continue trying to move the second MASK_STORE into the same >> conditional block, because it has the same mask. In this case it is wrong, >> because there is >> the scalar load in between (_24 = *_13) that just waits for DCE, but >> generally >> there could be arbitrary code. >> /* Put other masked stores with the same mask to STORE_BB. */ >> if (worklist.is_empty () >> || gimple_call_arg (worklist.last (), 2) != mask >> || worklist.last () != stmt1) >> break; >> has a simplistic check (doesn't consider other MASK_STORE unless the walking >> walked up to that stmt), but of course it doesn't work too well if some >> scalar >> stmts were skipped. >> >> I see various issues in that function: >> 1) wrong formatting: >> gsi_to = gsi_start_bb (store_bb); >> if (dump_enabled_p ()) >> { >> dump_printf_loc (MSG_NOTE, vect_location, >>"Move stmt to created bb\n"); >> dump_gimple_stmt (MSG_NOTE, TDF_SLIM, last, 0); >> } >> /* Move all stored value producers if possible. */ >> while (!gsi_end_p (gsi)) >> { >> The Move all stored value and everything below up to corresponding closing } >> should be moved two columns to the left >> 2) IMHO stmt1 should be set to NULL before that while (!gsi_end_p (gsi)), >> as the function is prepared to handle multiple bbs >> 3) next to gimple_vdef non-NULL break IMHO should be also >> gimple_has_volatile_ops -> break check, just for safety, we don't wanto to >> mishandle say volatile reads etc. >> 4) you have to skip over debug stmts if there are any, otherwise we have a >> -fcompare-debug issue >> 5) IMHO you should give up also for !is_gimple_assign, say trying to move an >> elemental function call into the conditional is just wrong >> 6) the >> /* Skip scalar statements. */ >> if (!VECTOR_TYPE_P (TREE_TYPE (lhs))) >> continue; >> should be reconsidered. IMHO if you have scalar stmts that feed just the >> stmts >> in the STORE_BB, there is no reason not to move them too, if you have scalar >> stmts that feed other stmts too, IMHO you should give up on them if they >> have a >> vuse; what code did you have in mind when adding the !VECTOR_TYPE_P check? >> 7) FOR_EACH_IMM_USE_FAST loop should ignore debug stmts, at least for >> decisions >> when to stop in some stmt; bet the debug stmts if there are any need to be >> reset >> if we move the def stmt that they are using, otherwise we risk >> -fcompare-debug >> issues >> 8) the worklist.last () != stmt1 check need to be -fcompare-debug friendly >> too, >> so if there are debug stmts in between the last moved stmt and the previous >> MASK_STORE, we need to handle it as if there aren't any debug stmts in >> between >> >> -- >> You are receiving this mail because: >> You are on the CC list for t
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 --- Comment #6 from Jakub Jelinek --- Well, MASK_STORE you don't want to handle in that loop, that is a store. MASK_LOAD is an exception, so IMHO you should just check for is_gimple_assign || MASK_LOAD. Allowing move of arbitrary other stmts looks dangerous. Another question is what to do with gimple_clobber_p. Those should have a vdef, so we handle them likely conservatively, which might be good enough for now. Or does vectorization just remove them already?
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 --- Comment #7 from rguenther at suse dot de --- On Fri, 5 Feb 2016, jakub at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 > > --- Comment #6 from Jakub Jelinek --- > Well, MASK_STORE you don't want to handle in that loop, that is a store. > MASK_LOAD is an exception, so IMHO you should just check for is_gimple_assign > || MASK_LOAD. Allowing move of arbitrary other stmts looks dangerous. Why? We track "dangerous" by having a VDEF. That's good enough IMHO. > Another question is what to do with gimple_clobber_p. Those should have a > vdef, so we handle them likely conservatively, which might be good enough for > now. Or does vectorization just remove them already? It does.
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 --- Comment #8 from Ilya Enkovich --- Author: ienkovich Date: Wed Feb 10 15:22:17 2016 New Revision: 233275 URL: https://gcc.gnu.org/viewcvs?rev=233275&root=gcc&view=rev Log: gcc/ 2016-02-10 Yuri Rumyantsev PR tree-optimization/69652 * tree-vect-loop.c (optimize_mask_stores): Move declaration of STMT1 to nested loop, did source re-formatting, skip debug statements, add check on statement with volatile operand, remove dead scalar statements. gcc/testsuite/ 2016-02-10 Yuri Rumyantsev PR tree-optimization/69652 * gcc.dg/torture/pr69652.c: New test. Added: trunk/gcc/testsuite/gcc.dg/torture/pr69652.c Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/tree-vect-loop.c
[Bug tree-optimization/69652] [6 Regression] [ICE] verify_ssa fail w/ -O2 -ffast-math -ftree-vectorize
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69652 Jakub Jelinek changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #9 from Jakub Jelinek --- Fixed.