From patchwork Fri Feb 11 14:38:46 2011
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [trans-mem] PR46567 again
From: Aldy Hernandez <aldyh@redhat.com>
X-Patchwork-Id: 82759
Message-Id: <4D5549F6.7070008@redhat.com>
To: Richard Henderson <rth@redhat.com>,
 Patrick MARLIER <patrick.marlier@unine.ch>,
 gcc-patches <gcc-patches@gcc.gnu.org>
Date: Fri, 11 Feb 2011 08:38:46 -0600

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.


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);
 	    }
 	}
     }