Patchwork Fix PR bootstrap/57154 (issue9179043)

login
register
mail settings
Submitter Teresa Johnson
Date May 3, 2013, 6:35 p.m.
Message ID <CAAe5K+VFYiE+fQ1Fc9sHN_V5bUuwCbgBZBy+rR3gvJfd77j=NA@mail.gmail.com>
Download mbox | patch
Permalink /patch/241366/
State New
Headers show

Comments

Teresa Johnson - May 3, 2013, 6:35 p.m.
Here is the patch with the new test case. Tested using dejagnu with
and without my fix for PR57154 to confirm that it exposes the failure
and works with the patch.

Ok for trunk?

Thanks,
Teresa

2013-05-03  Teresa Johnson  <tejohnson@google.com>

PR bootstrap/57154
* gcc.dg/pr57154.c: New test.


On Fri, May 3, 2013 at 10:34 AM, Teresa Johnson <tejohnson@google.com> wrote:
> On Fri, May 3, 2013 at 10:28 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
>> On Fri, May 3, 2013 at 1:26 PM, Jeff Law <law@redhat.com> wrote:
>>> On 05/03/2013 11:15 AM, Teresa Johnson wrote:
>>>>
>>>> On Fri, May 3, 2013 at 9:37 AM, David Edelsohn <dje.gcc@gmail.com> wrote:
>>>>>
>>>>> Thanks for the patch.
>>>>>
>>>>> Bootstrap works on x86 and Anton's testcase works on x86.
>>>>>
>>>>> Is there any testcase for x86 that would demonstrate the failure or
>>>>> that could check the probabilities in a dump file and see the
>>>>> inconsistency?
>>>>
>>>>
>>>> Patch is committed. But I just figured out why it isn't showing up on
>>>> x86 - it is because -fschedule-insns is not enabled by default. I can
>>>> reproduce it with x86 using the same test case. Will go ahead and
>>>> submit a patch to add this to the testsuite.
>>>
>>> Just an FYI, we don't use -fschedule-insns (first pass) on x86/x86_64 due to
>>> register pressure issues.   However, I think for the testsuite it'd be OK to
>>> have a test which uses -fschedule-insns to expose the need for clamping on
>>> the probabilities.
>>
>> Then Anton's testcase would make a good addition, with explicit
>> -fschedule-insns.
>
> Yep, getting that patch ready right now.
> Teresa
>
>>
>> Thanks, David
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Jeff Law - May 3, 2013, 6:47 p.m.
On 05/03/2013 12:35 PM, Teresa Johnson wrote:
> Here is the patch with the new test case. Tested using dejagnu with
> and without my fix for PR57154 to confirm that it exposes the failure
> and works with the patch.
>
> Ok for trunk?
>
> Thanks,
> Teresa
>
> 2013-05-03  Teresa Johnson  <tejohnson@google.com>
>
> PR bootstrap/57154
> * gcc.dg/pr57154.c: New test.
Is the test just supposed to ICE or something similar when it fails? 
Any reason not to put it into c-torture so that it gets run with 
multiple option sets?  You can use the same dg-options line to force on 
the scheduler.

No objection to it being in gcc.dg though.

jeff
Teresa Johnson - May 3, 2013, 10:46 p.m.
Resending since it bounced as my mailer wasn't set to plain text.
Teresa

On Fri, May 3, 2013 at 12:48 PM, Teresa Johnson <tejohnson@google.com> wrote:
> Yes it will ICE on failure. What is the guideline on c.torture vs gcc.dg?
> Thanks,
> Teresa
>
> On May 3, 2013 11:47 AM, "Jeff Law" <law@redhat.com> wrote:
>>
>> On 05/03/2013 12:35 PM, Teresa Johnson wrote:
>>>
>>> Here is the patch with the new test case. Tested using dejagnu with
>>> and without my fix for PR57154 to confirm that it exposes the failure
>>> and works with the patch.
>>>
>>> Ok for trunk?
>>>
>>> Thanks,
>>> Teresa
>>>
>>> 2013-05-03  Teresa Johnson  <tejohnson@google.com>
>>>
>>> PR bootstrap/57154
>>> * gcc.dg/pr57154.c: New test.
>>
>> Is the test just supposed to ICE or something similar when it fails? Any
>> reason not to put it into c-torture so that it gets run with multiple option
>> sets?  You can use the same dg-options line to force on the scheduler.
>>
>> No objection to it being in gcc.dg though.
>>
>> jeff
>>



--
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Jeff Law - May 6, 2013, 3:24 p.m.
On 05/03/2013 04:46 PM, Teresa Johnson wrote:
> On Fri, May 3, 2013 at 12:48 PM, Teresa Johnson <tejohnson@google.com> wrote:
>> Yes it will ICE on failure. What is the guideline on c.torture vs gcc.dg?
I don't think there's any general guidelines.

c-torture was an older framework that was considerably less expressive 
in terms of control of flags, testing for specific messages, etc.  But 
c-torture had the advantage that it iterates through a (predefined) list 
of options, testing each one individually while gcc.dg ran each test a 
single time.

A many years ago parts of the older c-torture framework were revamped to 
utilize the gcc.dg framework *but* they kept the ability to run the 
tests with a variety of options.

Based on my experience I tend to prefer the torture framework as it 
gives coverage across a wider variety of options and that's proven 
useful through the years.  For this particular test the increase in 
coverage is marginal, hence my comment "No objection to it being in 
gcc.dg though".

jeff
Jakub Jelinek - May 6, 2013, 3:29 p.m.
On Mon, May 06, 2013 at 09:24:28AM -0600, Jeff Law wrote:
> On 05/03/2013 04:46 PM, Teresa Johnson wrote:
> >On Fri, May 3, 2013 at 12:48 PM, Teresa Johnson <tejohnson@google.com> wrote:
> >>Yes it will ICE on failure. What is the guideline on c.torture vs gcc.dg?
> I don't think there's any general guidelines.
> 
> c-torture was an older framework that was considerably less
> expressive in terms of control of flags, testing for specific
> messages, etc.  But c-torture had the advantage that it iterates
> through a (predefined) list of options, testing each one
> individually while gcc.dg ran each test a single time.
> 
> A many years ago parts of the older c-torture framework were
> revamped to utilize the gcc.dg framework *but* they kept the ability
> to run the tests with a variety of options.
> 
> Based on my experience I tend to prefer the torture framework as it
> gives coverage across a wider variety of options and that's proven
> useful through the years.  For this particular test the increase in
> coverage is marginal, hence my comment "No objection to it being in
> gcc.dg though".

Note that there is also gcc.dg/torture/ which also runs multiple options.

	Jakub
Teresa Johnson - May 8, 2013, 1:47 a.m.
Thanks for the background. I had gone ahead and put it into gcc.dg,
but next time I can put it in gcc.dg/torture.

Teresa

On Mon, May 6, 2013 at 8:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, May 06, 2013 at 09:24:28AM -0600, Jeff Law wrote:
>> On 05/03/2013 04:46 PM, Teresa Johnson wrote:
>> >On Fri, May 3, 2013 at 12:48 PM, Teresa Johnson <tejohnson@google.com> wrote:
>> >>Yes it will ICE on failure. What is the guideline on c.torture vs gcc.dg?
>> I don't think there's any general guidelines.
>>
>> c-torture was an older framework that was considerably less
>> expressive in terms of control of flags, testing for specific
>> messages, etc.  But c-torture had the advantage that it iterates
>> through a (predefined) list of options, testing each one
>> individually while gcc.dg ran each test a single time.
>>
>> A many years ago parts of the older c-torture framework were
>> revamped to utilize the gcc.dg framework *but* they kept the ability
>> to run the tests with a variety of options.
>>
>> Based on my experience I tend to prefer the torture framework as it
>> gives coverage across a wider variety of options and that's proven
>> useful through the years.  For this particular test the increase in
>> coverage is marginal, hence my comment "No objection to it being in
>> gcc.dg though".
>
> Note that there is also gcc.dg/torture/ which also runs multiple options.
>
>         Jakub

Patch

Index: gcc.dg/pr57154.c
===================================================================
--- gcc.dg/pr57154.c (revision 0)
+++ gcc.dg/pr57154.c (revision 0)
@@ -0,0 +1,43 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fschedule-insns" } */
+
+#define PF_FROZEN 0x00010000
+#define likely(x)      __builtin_expect(!!(x), 1)
+
+struct cur
+{
+  unsigned long flags;
+};
+struct cur *cur;
+
+unsigned long freeze_cnt;
+
+extern int foo(void *);
+extern int slow_path(void *);
+
+static inline int freezing(void *p)
+{
+        if (likely(!foo(&freeze_cnt)))
+                return 0;
+        return slow_path(p);
+}
+
+extern int blah(void);
+
+int testcase(int check_kthr_stop)
+{
+  int was_frozen = 0;
+
+  for (;;) {
+    if (!freezing(cur) ||
+        (check_kthr_stop && blah()))
+      cur->flags &= ~PF_FROZEN;
+
+    if (!(cur->flags & PF_FROZEN))
+      break;
+
+    was_frozen = 1;
+  }
+
+  return was_frozen;
+}