Patchwork [committed] Fix PR 57422

login
register
mail settings
Submitter Andrey Belevantsev
Date Dec. 23, 2013, 6:52 a.m.
Message ID <52B7DDA9.4000308@ispras.ru>
Download mbox | patch
Permalink /patch/304708/
State New
Headers show

Comments

Andrey Belevantsev - Dec. 23, 2013, 6:52 a.m.
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?

Andrey
Jakub Jelinek - Dec. 23, 2013, 8:08 a.m.
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
H.J. Lu - Dec. 23, 2013, 12:24 p.m.
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?
Andrey Belevantsev - Dec. 27, 2013, 10:11 a.m.
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
Jakub Jelinek - Dec. 27, 2013, 10:16 a.m.
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
Jeff Law - Jan. 6, 2014, 4:52 p.m.
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
Andrey Belevantsev - Jan. 21, 2014, 12:36 p.m.
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

Patch

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,