This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Bug c++/46269] [trans-mem] internal compiler error in expand_block_tm of trans-mem.c
On Fri, Nov 12, 2010 at 10:13:02AM -0800, Richard Henderson wrote:
> On 11/12/2010 06:24 AM, Aldy Hernandez wrote:
> > - if (gimple_code (g) == GIMPLE_CALL)
> > + if (gimple_code (g) == GIMPLE_ASM && region->irr_blocks)
> > + bitmap_set_bit (region->irr_blocks, bb->index);
>
> This is wrong. An ASM does not transition to irrevocable,
I won't undignify myself with a response to the rest of this email.
Suffice to say that I'm an idiot. I should've quit while I was ahead
and left the implementation to the interested reader...
> This is a bug elsewhere. E.g.
>
> > if (is_tm_irrevocable (node->decl)
> > || (a >= AVAIL_OVERWRITABLE
> > && !tree_versionable_function_p (node->decl)))
> > ipa_tm_note_irrevocable (node, &worklist);
>
> if (is_tm_irrevocable (node->decl)
> || a <= AVAIL_NOT_AVAILABLE
> || !tree_versionable_function_p (node->decl))
> ipa_tm_note_irrevocable (node, &worklist);
This was actually the root of most everything. No need to initialize
the worklist, if we will actually fill it with correct info in the first
place :).
Of course, this opened up all sorts of grief. For instance, we can't
just mark not available functions as irrevocable, because they may be
explicitly marked as safe or pure. On the other hand, callers of
(assumed) irrevocable functions should not force the irrevocability bits
up the call chain, if the (assumed) irrevocable function was
specifically marked as safe/pure. The latter problem can be seen in
c-c++-common/tm/safe-3.c.
Anyways, look at the patch now. Feel free to continue crucifying me if
I got it wrong again.
Tested on x86-64 Linux.
* gimple-pretty-print.c (dump_gimple_call): Add clone attribute to
dump.
* trans-mem.c (tm_region_init_1): Proceed even if there are not
exit_blocks.
(gate_tm_init): Initialize irr_blocks.
(execute_tm_mark): Stop at irrevocable blocks for clones too.
(ipa_tm_note_irrevocable): Do not mark callers as irrevocable if
they are marked safe or pure.
(ipa_tm_scan_irr_function): Only dump irrevocable blocks if we
have them.
(ipa_tm_execute): Rework irrevocable marking logic.
PR/46269
* trans-mem.c (expand_block_tm): Handle GIMPLE_ASM.
Index: testsuite/g++.dg/tm/pr46269.C
===================================================================
--- testsuite/g++.dg/tm/pr46269.C (revision 0)
+++ testsuite/g++.dg/tm/pr46269.C (revision 0)
@@ -0,0 +1,29 @@
+// { dg-do compile }
+// { dg-options "-fgnu-tm" }
+
+static inline void atomic_exchange_and_add()
+{
+ __asm__ ("blah");
+}
+
+template<class T> class shared_ptr
+{
+public:
+ shared_ptr( T * p )
+ {
+ atomic_exchange_and_add();
+ }
+};
+
+class BuildingCompletedEvent
+{
+ public:
+ __attribute__((transaction_callable)) void updateBuildingSite(void);
+ __attribute__((transaction_pure)) BuildingCompletedEvent();
+};
+
+void BuildingCompletedEvent::updateBuildingSite(void)
+{
+ shared_ptr<BuildingCompletedEvent> event(new BuildingCompletedEvent());
+}
+
Index: gimple-pretty-print.c
===================================================================
--- gimple-pretty-print.c (revision 166496)
+++ gimple-pretty-print.c (working copy)
@@ -541,6 +541,8 @@ dump_gimple_call (pretty_printer *buffer
/* Dump the arguments of _ITM_beginTransaction sanely. */
if (TREE_CODE (fn) == ADDR_EXPR)
fn = TREE_OPERAND (fn, 0);
+ if (TREE_CODE (fn) == FUNCTION_DECL && DECL_IS_TM_CLONE (fn))
+ pp_string (buffer, " [tm-clone]");
if (TREE_CODE (fn) == FUNCTION_DECL
&& DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL
&& DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_START
Index: trans-mem.c
===================================================================
--- trans-mem.c (revision 166604)
+++ trans-mem.c (working copy)
@@ -1732,7 +1732,8 @@ tm_region_init_1 (struct tm_region *regi
gimple_stmt_iterator gsi;
gimple g;
- if (!region || !region->exit_blocks)
+ if (!region
+ || (!region->irr_blocks && !region->exit_blocks))
return region;
/* Check to see if this is the end of a region by seeing if it
@@ -1746,8 +1747,9 @@ tm_region_init_1 (struct tm_region *regi
tree fn = gimple_call_fndecl (g);
if (fn && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL)
{
- if (DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT
- || DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT_EH)
+ if ((DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT
+ || DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_COMMIT_EH)
+ && region->exit_blocks)
{
bitmap_set_bit (region->exit_blocks, bb->index);
region = region->outer;
@@ -1831,6 +1833,10 @@ gate_tm_init (void)
obstack_alloc (&tm_obstack.obstack, sizeof (struct tm_region));
memset (region, 0, sizeof (*region));
region->entry_block = single_succ (ENTRY_BLOCK_PTR);
+ /* For a clone, the entire function is the region. But even if
+ we don't need to record any exit blocks, we may need to
+ record irrevocable blocks. */
+ region->irr_blocks = BITMAP_ALLOC (&tm_obstack);
tm_region_init (region);
}
@@ -2285,6 +2291,7 @@ execute_tm_mark (void)
for (region = all_tm_regions; region ; region = region->next)
{
tm_log_init ();
+ /* If we have a transaction... */
if (region->exit_blocks)
{
unsigned int subcode
@@ -2307,9 +2314,17 @@ execute_tm_mark (void)
VEC_free (basic_block, heap, queue);
}
else
+ /* ...otherwise, we're a clone and the entire function is the
+ region. */
{
FOR_EACH_BB (bb)
- expand_block_tm (region, bb);
+ {
+ /* Stop at irrevocable blocks. */
+ if (region->irr_blocks
+ && bitmap_bit_p (region->irr_blocks, bb->index))
+ break;
+ expand_block_tm (region, bb);
+ }
}
tm_log_emit ();
}
@@ -3452,6 +3467,11 @@ ipa_tm_note_irrevocable (struct cgraph_n
/* Don't examine recursive calls. */
if (e->caller == node)
continue;
+ /* Even if we think we can go irrevocable, believe the user
+ above all. */
+ if (is_tm_safe (e->caller->decl)
+ || is_tm_pure (e->caller->decl))
+ continue;
if (gimple_call_in_transaction_p (e->call_stmt))
d->want_irr_scan_normal = true;
maybe_push_queue (e->caller, worklist_p, &d->in_worklist);
@@ -3714,21 +3734,21 @@ ipa_tm_scan_irr_function (struct cgraph_
d->irrevocable_blocks_clone = new_irr;
else
d->irrevocable_blocks_normal = new_irr;
+
+ if (dump_file)
+ {
+ const char *dname;
+ bitmap_iterator bmi;
+ unsigned i;
+
+ dname = lang_hooks.decl_printable_name (current_function_decl, 2);
+ EXECUTE_IF_SET_IN_BITMAP (new_irr, 0, i, bmi)
+ fprintf (dump_file, "%s: bb %d goes irrevocable\n", dname, i);
+ }
}
else
BITMAP_FREE (new_irr);
- if (dump_file)
- {
- const char *dname;
- bitmap_iterator bmi;
- unsigned i;
-
- dname = lang_hooks.decl_printable_name (current_function_decl, 2);
- EXECUTE_IF_SET_IN_BITMAP (new_irr, 0, i, bmi)
- fprintf (dump_file, "%s: bb %d goes irrevocable\n", dname, i);
- }
-
VEC_free (basic_block, heap, queue);
pop_cfun ();
current_function_decl = NULL;
@@ -4394,12 +4414,19 @@ ipa_tm_execute (void)
/* Some callees cannot be arbitrarily cloned. These will always be
irrevocable. Mark these now, so that we need not scan them. */
- if (is_tm_irrevocable (node->decl)
- || (a >= AVAIL_OVERWRITABLE
- && !tree_versionable_function_p (node->decl)))
+ if (is_tm_irrevocable (node->decl))
+ ipa_tm_note_irrevocable (node, &worklist);
+ else if (a <= AVAIL_NOT_AVAILABLE
+ && !is_tm_safe (node->decl)
+ && !is_tm_pure (node->decl))
ipa_tm_note_irrevocable (node, &worklist);
- else if (a >= AVAIL_OVERWRITABLE && !d->is_irrevocable)
- ipa_tm_scan_calls_clone (node, &tm_callees);
+ else if (a >= AVAIL_OVERWRITABLE)
+ {
+ if (!tree_versionable_function_p (node->decl))
+ ipa_tm_note_irrevocable (node, &worklist);
+ else if (!d->is_irrevocable)
+ ipa_tm_scan_calls_clone (node, &tm_callees);
+ }
}
/* Iterate scans until no more work to be done. Prefer not to use