Patchwork ifcvt/crossjump patch: Fix PR 42496, 21803

login
register
mail settings
Submitter Bernd Schmidt
Date Aug. 3, 2010, 3:31 p.m.
Message ID <4C583639.9070704@codesourcery.com>
Download mbox | patch
Permalink /patch/60770/
State New
Headers show

Comments

Bernd Schmidt - Aug. 3, 2010, 3:31 p.m.
On 08/03/2010 05:15 PM, Jeff Law wrote:
>  On 08/03/10 08:09, Bernd Schmidt wrote:
>> On 08/02/2010 06:15 PM, Bernd Schmidt wrote:
>>> On 08/02/2010 06:05 PM, Jeff Law wrote:
>>>> OK.  If you could highlight in a quick blurb what changed it'd be
>>>> appreciated -- it'll save me from having to look over the whole thing
>>>> again to figure out what changed from the previous version.
>>> I intend to make the change I previously mentioned to add a per-bb flag
>>> which notes it's been modified, so that we can use that on the second
>>> pass to decide whether or not to try to optimize it, rather than using
>>> df_get_bb_dirty (since that gets cleared on df_analyze).  Earlier
>>> versions of gcc had a BB_DIRTY bit in bb->flags, I'll reintroduce that
>>> as BB_MODIFIED.  That's cheaper to test anyway.
>>>
>>> The other change I'll make is to be slightly more careful wrt. volatile
>>> asms, not moving memory references across them.
>> Did that, and also fixed a crash I saw with a PPC cross compiler -
>> mustn't try to look at insns in EXIT_BLOCK.  Note that there's still a
>> call to clear_bb_flags which I think is left over from before we were
>> using df_get_bb_dirty and now has a purpose again.
>>
>> New patch below; search for BB_MODIFIED, ASM_OPERANDS and EXIT_BLOCK_PTR
>> to find these changes.  Also, added the two testcases for i386 as well
>> and Paolo's suggestion of a gcc_assert before df_analyze.
>>
>> Bootstrapped and regression tested on i686-linux.
>>
>>
>> Bernd
> The testsuite changes weren't attached to the patch.

quilt vs svn problem; now svn added them.

> I guess one could ask whether or not we really need to carry a bit in
> the BB structure if its only use is head merging -- we could just as
> easily have a bitmap indicating what blocks changed that we allocate &
> free within cfgcleanup.

We have a flags word anyway, so I think using that is the simplest way.
 That way we also don't have to worry about BB numbers being stable.
I'm open to suggestions for a better comment.

> I don't see anything which ever clears BB_MODIFIED, so that bit is going
> to accumulate in the block over time.

That's what I meant when I mentioned the call to clear_bb_flags above.


Bernd
Jeff Law - Aug. 3, 2010, 5:12 p.m.
On 08/03/10 09:31, Bernd Schmidt wrote:
> On 08/03/2010 05:15 PM, Jeff Law wrote:
>>   On 08/03/10 08:09, Bernd Schmidt wrote:
>>> On 08/02/2010 06:15 PM, Bernd Schmidt wrote:
>>>> On 08/02/2010 06:05 PM, Jeff Law wrote:
>>>>> OK.  If you could highlight in a quick blurb what changed it'd be
>>>>> appreciated -- it'll save me from having to look over the whole thing
>>>>> again to figure out what changed from the previous version.
>>>> I intend to make the change I previously mentioned to add a per-bb flag
>>>> which notes it's been modified, so that we can use that on the second
>>>> pass to decide whether or not to try to optimize it, rather than using
>>>> df_get_bb_dirty (since that gets cleared on df_analyze).  Earlier
>>>> versions of gcc had a BB_DIRTY bit in bb->flags, I'll reintroduce that
>>>> as BB_MODIFIED.  That's cheaper to test anyway.
>>>>
>>>> The other change I'll make is to be slightly more careful wrt. volatile
>>>> asms, not moving memory references across them.
>>> Did that, and also fixed a crash I saw with a PPC cross compiler -
>>> mustn't try to look at insns in EXIT_BLOCK.  Note that there's still a
>>> call to clear_bb_flags which I think is left over from before we were
>>> using df_get_bb_dirty and now has a purpose again.
>>>
>>> New patch below; search for BB_MODIFIED, ASM_OPERANDS and EXIT_BLOCK_PTR
>>> to find these changes.  Also, added the two testcases for i386 as well
>>> and Paolo's suggestion of a gcc_assert before df_analyze.
>>>
>>> Bootstrapped and regression tested on i686-linux.
>>>
>>>
>>> Bernd
>> The testsuite changes weren't attached to the patch.
> quilt vs svn problem; now svn added them.
np.  They're clearly not the meat of the patch :-)

>> I guess one could ask whether or not we really need to carry a bit in
>> the BB structure if its only use is head merging -- we could just as
>> easily have a bitmap indicating what blocks changed that we allocate&
>> free within cfgcleanup.
> We have a flags word anyway, so I think using that is the simplest way.
>   That way we also don't have to worry about BB numbers being stable.
> I'm open to suggestions for a better comment.
OK.  WRT the comment, we might want to just say that BB_MODIFIED is set 
at the same time as a block is marked dirty, but is not cleared during a 
df_analyze allowing a pass to update the DF information and still know 
what blocks were modified.

>> I don't see anything which ever clears BB_MODIFIED, so that bit is going
>> to accumulate in the block over time.
> That's what I meant when I mentioned the call to clear_bb_flags above.
Duh.  I shouldn't review code first thing in the morning.

jeff

Patch

Index: testsuite/gcc.target/arm/headmerge-1.c
===================================================================
--- testsuite/gcc.target/arm/headmerge-1.c	(revision 0)
+++ testsuite/gcc.target/arm/headmerge-1.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile }  */
+/* { dg-options "-O2" }  */
+/* { dg-final { scan-assembler-times "#120" 1 } } */
+
+extern void foo1 (int);
+extern void foo2 (int);
+
+void t (int x, int y)
+{
+  if (y < 5)
+    foo1 (120);
+  else
+    foo2 (120);
+}
Index: testsuite/gcc.target/arm/headmerge-2.c
===================================================================
--- testsuite/gcc.target/arm/headmerge-2.c	(revision 0)
+++ testsuite/gcc.target/arm/headmerge-2.c	(revision 0)
@@ -0,0 +1,35 @@ 
+/* { dg-do compile }  */
+/* { dg-options "-O2" }  */
+/* { dg-final { scan-assembler-times "120" 1 } } */
+
+extern void foo1 (int);
+extern void foo2 (int);
+extern void foo3 (int);
+extern void foo4 (int);
+extern void foo5 (int);
+extern void foo6 (int);
+
+void t (int x, int y)
+{
+  switch (y)
+    {
+    case 1:
+      foo1 (120);
+      break;
+    case 5:
+      foo2 (120);
+      break;
+    case 7:
+      foo3 (120);
+      break;
+    case 10:
+      foo4 (120);
+      break;
+    case 13:
+      foo5 (120);
+      break;
+    default:
+      foo6 (120);
+      break;
+    }
+}
Index: testsuite/gcc.target/i386/headmerge-1.c
===================================================================
--- testsuite/gcc.target/i386/headmerge-1.c	(revision 0)
+++ testsuite/gcc.target/i386/headmerge-1.c	(revision 0)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile }  */
+/* { dg-options "-O2" }  */
+/* { dg-final { scan-assembler-times "120" 1 } } */
+
+extern void foo1 (int);
+extern void foo2 (int);
+
+void t (int x, int y)
+{
+  if (y < 5)
+    foo1 (120);
+  else
+    foo2 (120);
+}
Index: testsuite/gcc.target/i386/headmerge-2.c
===================================================================
--- testsuite/gcc.target/i386/headmerge-2.c	(revision 0)
+++ testsuite/gcc.target/i386/headmerge-2.c	(revision 0)
@@ -0,0 +1,35 @@ 
+/* { dg-do compile }  */
+/* { dg-options "-O2" }  */
+/* { dg-final { scan-assembler-times "120" 1 } } */
+
+extern void foo1 (int);
+extern void foo2 (int);
+extern void foo3 (int);
+extern void foo4 (int);
+extern void foo5 (int);
+extern void foo6 (int);
+
+void t (int x, int y)
+{
+  switch (y)
+    {
+    case 1:
+      foo1 (120);
+      break;
+    case 5:
+      foo2 (120);
+      break;
+    case 7:
+      foo3 (120);
+      break;
+    case 10:
+      foo4 (120);
+      break;
+    case 13:
+      foo5 (120);
+      break;
+    default:
+      foo6 (120);
+      break;
+    }
+}