Patchwork [trans-mem] PR46567 again

login
register
mail settings
Submitter Aldy Hernandez
Date Feb. 11, 2011, 2:38 p.m.
Message ID <4D5549F6.7070008@redhat.com>
Download mbox | patch
Permalink /patch/82759/
State New
Headers show

Comments

Aldy Hernandez - Feb. 11, 2011, 2:38 p.m.
In tm_ipa_execute we iterate over the irr_worklist until no more work is 
left, which means, that ipa_tm_propagate_irr() can be called multiple 
for a given function.

#ifdef __VERBOSE
In the testcase below, we analyze readLoop(), noting that the call to 
funky() will go irrevocable, but since we have no hard evidence on 
SeqfileGetLine(), we don't know its BB will also go irrevocable.

Then we analyze SeqfileGetLine(), at which point we know it will go 
irrevocable, and we proceed to add its callers (readLoop() again) to the 
worklist.

When we analyze readLoop() again, we notice right away that the call to 
SeqfileGetLine() will go irrevocable, so we proceed to propagate the 
irrevocability to the immediate dominators.  However, since one of the 
immediate dominators in readLoop() is the call to funky() which we've 
already processed, we die in the assert.

This assert should be an if, like the similar if a few lines up.
#else
The assert should be an if, because we can iterate multiple times 
through the same function and end up analyzing an immediate dominator 
which we already analyzed.
#endif

Had it not taken me an entire day to figure this out, I would've 
ventured to say this is an obvious fix.  Instead, I'll patiently wait 
for approval.

OK for branch?

p.s. Finally... We've fixed all the known failures in this PR.
PR 46567
	* trans-mem.c (ipa_tm_propagate_irr): Change assert to if.
Richard Henderson - Feb. 11, 2011, 6:11 p.m.
On 02/11/2011 06:38 AM, Aldy Hernandez wrote:
> 	PR 46567
> 	* trans-mem.c (ipa_tm_propagate_irr): Change assert to if.

Ok.


r~

Patch

Index: testsuite/gcc.dg/tm/pr46567-2.c
===================================================================
--- testsuite/gcc.dg/tm/pr46567-2.c	(revision 0)
+++ testsuite/gcc.dg/tm/pr46567-2.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm" } */
+
+int funky();
+int global;
+
+void SeqfileGetLine()
+{
+  funky();
+}
+
+__attribute__((transaction_callable)) void readLoop()
+{
+        SeqfileGetLine();
+        if (global)
+          funky();
+
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 170050)
+++ trans-mem.c	(working copy)
@@ -3753,8 +3753,8 @@  ipa_tm_propagate_irr (basic_block entry_
 	       son = next_dom_son (CDI_DOMINATORS, son))
 	    {
 	      /* Make sure a block isn't already in old_irr.  */
-	      gcc_assert (!old_irr || !bitmap_bit_p (old_irr, son->index));
-	      bitmap_set_bit (new_irr, son->index);
+	      if (!old_irr || !bitmap_bit_p (old_irr, son->index))
+		bitmap_set_bit (new_irr, son->index);
 	    }
 	}
     }