| Message ID | 52B7DDA9.4000308@ispras.ru |
|---|---|
| State | New |
| Headers | show |
On Mon, Dec 23, 2013 at 10:52:25AM +0400, Andrey Belevantsev wrote: > As described in the PR, the ICE reason was the typo made when > introducing calls to add_hard_reg_set. Fixed by the first attached > patch, bootstrapped and tested on both ia64 and x86_64, committed as > obvious. > > The test case is very sensitive to the scheduler decisions (e.g. it > didn't fail on trunk but only on the revision reported for me), so > instead of adding the test I have put in the code two asserts > checking that we can always schedule the fence instruction as is. > This hunk was tested together with the first but committed > separately. > > The first patch can be safely committed to 4.8, the second can stay > on trunk only. Jakub, will it be fine with you? Yes. Jakub
On Sun, Dec 22, 2013 at 10:52 PM, Andrey Belevantsev <abel@ispras.ru> wrote: > Hello, > > As described in the PR, the ICE reason was the typo made when introducing > calls to add_hard_reg_set. Fixed by the first attached patch, bootstrapped > and tested on both ia64 and x86_64, committed as obvious. > > The test case is very sensitive to the scheduler decisions (e.g. it didn't > fail on trunk but only on the revision reported for me), so instead of > adding the test I have put in the code two asserts checking that we can > always schedule the fence instruction as is. This hunk was tested together > with the first but committed separately. > Testcase is very small. Why not add it?
On 23.12.2013 16:24, H.J. Lu wrote: > On Sun, Dec 22, 2013 at 10:52 PM, Andrey Belevantsev <abel@ispras.ru> wrote: >> Hello, >> >> As described in the PR, the ICE reason was the typo made when introducing >> calls to add_hard_reg_set. Fixed by the first attached patch, bootstrapped >> and tested on both ia64 and x86_64, committed as obvious. >> >> The test case is very sensitive to the scheduler decisions (e.g. it didn't >> fail on trunk but only on the revision reported for me), so instead of >> adding the test I have put in the code two asserts checking that we can >> always schedule the fence instruction as is. This hunk was tested together >> with the first but committed separately. >> > > Testcase is very small. Why not add it? Frankly, I think that the chances of this test uncovering similar issues in the future are very small. It needs lots of options to make it trigger and even with this a specific revision. The chance of triggering the asserts I added on another code is much higher. In the past, I have also avoided to add tests that require 5+ options to trigger the issue, adding only those that have some hope on more ore less reliably reproducing the required issue. The best solution of course is having an infrastructure to test the specific scheduler decisions, which we don't have. You are welcome to add the test if you feel so strongly about us needing it. Andrey
On Fri, Dec 27, 2013 at 02:11:13PM +0400, Andrey Belevantsev wrote: > >Testcase is very small. Why not add it? > > Frankly, I think that the chances of this test uncovering similar > issues in the future are very small. It needs lots of options to > make it trigger and even with this a specific revision. The chance > of triggering the asserts I added on another code is much higher. > In the past, I have also avoided to add tests that require 5+ > options to trigger the issue, adding only those that have some hope > on more ore less reliably reproducing the required issue. The best > solution of course is having an infrastructure to test the specific > scheduler decisions, which we don't have. > > You are welcome to add the test if you feel so strongly about us needing it. I guess it depends, if you e.g. have a small runtime testcase, it might be useful to add it, while it is unlikely it will trigger the same issue, it could trigger a different issue in another part of the compiler, especially if the testcase is a combination of e.g. several more rarely used features. But for a ICE testcase with many weird options to trigger it I agree it sometimes doesn't make sense to add the testcase, especially if it already doesn't trigger on the trunk as in this case. Jakub
On 12/27/13 03:16, Jakub Jelinek wrote: > On Fri, Dec 27, 2013 at 02:11:13PM +0400, Andrey Belevantsev wrote: >>> Testcase is very small. Why not add it? >> >> Frankly, I think that the chances of this test uncovering similar >> issues in the future are very small. It needs lots of options to >> make it trigger and even with this a specific revision. The chance >> of triggering the asserts I added on another code is much higher. >> In the past, I have also avoided to add tests that require 5+ >> options to trigger the issue, adding only those that have some hope >> on more ore less reliably reproducing the required issue. The best >> solution of course is having an infrastructure to test the specific >> scheduler decisions, which we don't have. >> >> You are welcome to add the test if you feel so strongly about us needing it. > > I guess it depends, if you e.g. have a small runtime testcase, it might be > useful to add it, while it is unlikely it will trigger the same issue, it > could trigger a different issue in another part of the compiler, especially > if the testcase is a combination of e.g. several more rarely used features. > But for a ICE testcase with many weird options to trigger it I agree it > sometimes doesn't make sense to add the testcase, especially if it already > doesn't trigger on the trunk as in this case. IIRC, for this particular bug it also heavily depended on the exact register allocation at a key point. So it could easily go latent on the trunk. jeff
On 23.12.2013 10:52, Andrey Belevantsev wrote: > Hello, > > As described in the PR, the ICE reason was the typo made when introducing > calls to add_hard_reg_set. Fixed by the first attached patch, bootstrapped > and tested on both ia64 and x86_64, committed as obvious. > > The test case is very sensitive to the scheduler decisions (e.g. it didn't > fail on trunk but only on the revision reported for me), so instead of > adding the test I have put in the code two asserts checking that we can > always schedule the fence instruction as is. This hunk was tested together > with the first but committed separately. > > The first patch can be safely committed to 4.8, the second can stay on > trunk only. Jakub, will it be fine with you? Now the first hunk is also committed to 4.8 and 4.7. Andrey
Index: gcc/ChangeLog =================================================================== *** gcc/ChangeLog (revision 206173) --- gcc/ChangeLog (revision 206174) *************** *** 1,6 **** --- 1,12 ---- 2013-12-23 Andrey Belevantsev <abel@ispras.ru> PR rtl-optimization/57422 + * sel-sched.c (fill_vec_av_set): Assert that the fence insn + can always be scheduled in its current form. + + 2013-12-23 Andrey Belevantsev <abel@ispras.ru> + + PR rtl-optimization/57422 * sel-sched.c (mark_unavailable_hard_regs): Fix typo when calling add_to_hard_reg_set. Index: gcc/sel-sched.c =================================================================== *** gcc/sel-sched.c (revision 206173) --- gcc/sel-sched.c (revision 206174) *************** fill_vec_av_set (av_set_t av, blist_t bn *** 3801,3806 **** --- 3801,3807 ---- signed char target_available; bool is_orig_reg_p = true; int need_cycles, new_prio; + bool fence_insn_p = INSN_UID (insn) == INSN_UID (FENCE_INSN (fence)); /* Don't allow any insns other than from SCHED_GROUP if we have one. */ if (FENCE_SCHED_NEXT (fence) && insn != FENCE_SCHED_NEXT (fence)) *************** fill_vec_av_set (av_set_t av, blist_t bn *** 3855,3863 **** if (sched_verbose >= 4) sel_print ("Expr %d has no suitable target register\n", INSN_UID (insn)); ! continue; } /* Filter expressions that need to be renamed or speculated when pipelining, because compensating register copies or speculation checks are likely to be placed near the beginning of the loop, --- 3856,3871 ---- if (sched_verbose >= 4) sel_print ("Expr %d has no suitable target register\n", INSN_UID (insn)); ! ! /* A fence insn should not get here. */ ! gcc_assert (!fence_insn_p); ! continue; } + /* At this point a fence insn should always be available. */ + gcc_assert (!fence_insn_p + || INSN_UID (FENCE_INSN (fence)) == INSN_UID (EXPR_INSN_RTX (expr))); + /* Filter expressions that need to be renamed or speculated when pipelining, because compensating register copies or speculation checks are likely to be placed near the beginning of the loop,