[trans-mem] PR 46567

[trans-mem] PR 46567

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]

After looking at this, I decided to try a 3rd option.

the routine 'get_tm_region_blocks' find all the exit and irrevocable 
blocks. In the process of calculating those, it has to visit every block 
in a transaction. It then throws out this visited bitmap.

I added a parameter which will set all the visited blocks in a bitmap as 
well.

ipa_tm_execute simply visits each TM region and sets this bitmap.  We 
end up with a bitmap which has a bit set for every BB in a transaction.

Now rather than checking the in_transaction bit or using a callgraph 
edge field, we simply need to check the bitmap for the BB the call is in.

At least on the surface this seems to work as the testcases all pass and 
the bug doesn't trigger the exception any more.

Have a look at the patch and tell me what I'm missing, there are lots of 
other things going on I don't understand :-)  In particular, there is 
what appears to be a hack in 'ipa_tm_insert_gettmclone_call' which I 
simply removed... since it didn't appear needed any more...

If it seems reasonable I'll add the testcase into the testsuite and  the 
patch as well.
I guess I could also remove  gimple_call_in_transaction_p and references 
as well eh?

Andrew


[-- Attachment #2: pr46567 --]
[-- Type: text/plain, Size: 8761 bytes --]

2011-01-21  Andrew MacLeod  <amacleod@redhat.com>

	PR/46567 
	* trans-mem.c (examine_call_tm): Remove gimple_call_set_in_transaction.
	(get_tm_region_blocks): Add new parameter for returning all blocks in 
	transactions.
	(execute_tm_mark, expand_regions_1, execute_tm_memopt): Add extra
	parameter to call.
	(bb_in_TM_region): New bitfield for BB's in tranactions.
	(ipa_tm_scan_calls_transaction): Check new bitfield.
	(ipa_tm_note_irrevocable): Check new bitfield.
	(ipa_tm_propagate_irr, ipa_tm_diagnose_transaction): Add extra param.
	(ipa_tm_insert_gettmclone_call): Remove set_transaction hack.
	(ipa_tm_execute): Build bb bitmap.

Index: trans-mem.c
===================================================================
*** trans-mem.c	(revision 168633)
--- trans-mem.c	(working copy)
*************** examine_call_tm (unsigned *state, gimple
*** 1515,1522 ****
    gimple stmt = gsi_stmt (*gsi);
    tree fn;
  
-   gimple_call_set_in_transaction (stmt, true);
- 
    if (is_tm_pure_call (stmt))
      return;
  
--- 1515,1520 ----
*************** static VEC (basic_block, heap) *
*** 2320,2325 ****
--- 2318,2324 ----
  get_tm_region_blocks (basic_block entry_block,
  		      bitmap exit_blocks,
  		      bitmap irr_blocks,
+ 		      bitmap all_region_blocks,
  		      bool stop_at_irrevocable_p)
  {
    VEC(basic_block, heap) *bbs = NULL;
*************** get_tm_region_blocks (basic_block entry_
*** 2354,2359 ****
--- 2353,2361 ----
      }
    while (i < VEC_length (basic_block, bbs));
  
+   if (all_region_blocks)
+     bitmap_ior_into (all_region_blocks, visited_blocks);
+ 
    BITMAP_FREE (visited_blocks);
    return bbs;
  }
*************** execute_tm_mark (void)
*** 2395,2400 ****
--- 2397,2403 ----
        queue = get_tm_region_blocks (region->entry_block,
  				    region->exit_blocks,
  				    region->irr_blocks,
+ 				    NULL,
  				    /*stop_at_irr_p=*/true);
        for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i)
  	expand_block_tm (region, bb);
*************** expand_regions_1 (struct tm_region *regi
*** 2629,2634 ****
--- 2632,2638 ----
        queue = get_tm_region_blocks (region->entry_block,
  				    region->exit_blocks,
  				    region->irr_blocks,
+ 				    NULL,
  				    /*stop_at_irr_p=*/false);
        expand_transaction (region);
        for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i)
*************** execute_tm_memopt (void)
*** 3299,3305 ****
        /* Save all BBs for the current region.  */
        bbs = get_tm_region_blocks (region->entry_block,
  				  region->exit_blocks,
! 				  region->irr_blocks, false);
  
        /* Collect all the memory operations.  */
        for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
--- 3303,3311 ----
        /* Save all BBs for the current region.  */
        bbs = get_tm_region_blocks (region->entry_block,
  				  region->exit_blocks,
! 				  region->irr_blocks, 
! 				  NULL, 
! 				  false);
  
        /* Collect all the memory operations.  */
        for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
*************** struct gimple_opt_pass pass_tm_memopt =
*** 3363,3369 ****
  	(a) For all local public functions marked tm_callable, push
  	    it onto the tm_callee queue.
  
! 	(b) For all local functions, scan for calls marked in_transaction.
  	    Push the caller and callee onto the tm_caller and tm_callee
  	    queues.  Count the number of callers for each callee.
  
--- 3369,3375 ----
  	(a) For all local public functions marked tm_callable, push
  	    it onto the tm_callee queue.
  
! 	(b) For all local functions, scan for calls in transaction blocks.
  	    Push the caller and callee onto the tm_caller and tm_callee
  	    queues.  Count the number of callers for each callee.
  
*************** DEF_VEC_ALLOC_P (cgraph_node_p, heap);
*** 3437,3442 ****
--- 3443,3450 ----
  
  typedef VEC (cgraph_node_p, heap) *cgraph_node_queue;
  
+ /* List of all BB's that are in transactional regions.  */
+ static bitmap bb_in_TM_region = NULL;
  
  /* Return the ipa data associated with NODE, allocating zeroed memory
     if necessary.  */
*************** ipa_tm_scan_calls_transaction (struct cg
*** 3482,3488 ****
    tree replacement;
  
    for (e = node->callees; e ; e = e->next_callee)
!     if (gimple_call_in_transaction_p (e->call_stmt))
        {
  	struct tm_ipa_cg_data *d;
  
--- 3490,3497 ----
    tree replacement;
  
    for (e = node->callees; e ; e = e->next_callee)
!     /* Check if the callee is in a transactional region. */ 
!     if (bitmap_bit_p (bb_in_TM_region, gimple_bb (e->call_stmt)->index))
        {
  	struct tm_ipa_cg_data *d;
  
*************** ipa_tm_note_irrevocable (struct cgraph_n
*** 3547,3553 ****
  	 above all.  */
        if (is_tm_safe_or_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);
      }
--- 3556,3565 ----
  	 above all.  */
        if (is_tm_safe_or_pure (e->caller->decl))
  	continue;
! 
!       gcc_assert (gimple_bb (e->call_stmt) != NULL);
!       /* Check if the callee is in a transactional region. */ 
!       if (bitmap_bit_p (bb_in_TM_region, gimple_bb (e->call_stmt)->index))
  	d->want_irr_scan_normal = true;
        maybe_push_queue (e->caller, worklist_p, &d->in_worklist);
      }
*************** ipa_tm_propagate_irr (basic_block entry_
*** 3670,3676 ****
    if (old_irr && bitmap_bit_p (old_irr, entry_block->index))
      return;
  
!   bbs = get_tm_region_blocks (entry_block, exit_blocks, NULL, false);
    do
      {
        basic_block bb = VEC_pop (basic_block, bbs);
--- 3682,3688 ----
    if (old_irr && bitmap_bit_p (old_irr, entry_block->index))
      return;
  
!   bbs = get_tm_region_blocks (entry_block, exit_blocks, NULL, NULL, false);
    do
      {
        basic_block bb = VEC_pop (basic_block, bbs);
*************** ipa_tm_diagnose_transaction (struct cgra
*** 3921,3927 ****
  	size_t i;
  
  	bbs = get_tm_region_blocks (r->entry_block, r->exit_blocks,
! 				    r->irr_blocks, false);
  
  	for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
  	  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
--- 3933,3939 ----
  	size_t i;
  
  	bbs = get_tm_region_blocks (r->entry_block, r->exit_blocks,
! 				    r->irr_blocks, NULL, false);
  
  	for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
  	  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
*************** ipa_tm_insert_gettmclone_call (struct cg
*** 4184,4198 ****
    ret = make_ssa_name (ret, g);
    gimple_call_set_lhs (g, ret);
  
-   /* ??? If we need to go irrevocable, we can fail the intermediate
-      commit and restart the transaction.  But representing that means
-      splitting this basic block, which means busting all of the bitmaps
-      we've put together, as well as the dominator tree.  Perhaps we
-      can get away with ignoring it, since the indirect function that
-      we're about to call should also have the back edge.  */
-   if (0 && !safe)
-     gimple_call_set_in_transaction (g, true);
- 
    gsi_insert_before (gsi, g, GSI_SAME_STMT);
  
    cgraph_create_edge (node, cgraph_node (gettm_fn), g, 0,
--- 4196,4201 ----
*************** ipa_tm_execute (void)
*** 4445,4450 ****
--- 4448,4468 ----
  #endif
  
    bitmap_obstack_initialize (&tm_obstack);
+   bb_in_TM_region = BITMAP_ALLOC (NULL);
+ 
+   /* Build a bitmap of all BB"s inside transaction regions.  */
+   for (node = cgraph_nodes; node; node = node->next)
+     if (node->reachable && node->lowered
+ 	&& cgraph_function_body_availability (node) >= AVAIL_OVERWRITABLE)
+       {
+ 	struct tm_region *regions;
+ 	regions = ipa_tm_region_init (node); 
+ 	for ( ; regions; regions = regions->next)
+ 	  {
+ 	    get_tm_region_blocks (regions->entry_block, NULL, NULL, 
+ 				  bb_in_TM_region, false);
+ 	  }
+       }
  
    /* For all local functions marked tm_callable, queue them.  */
    for (node = cgraph_nodes; node; node = node->next)
*************** ipa_tm_execute (void)
*** 4472,4478 ****
  	    continue;
  	  }
  
! 	/* ... otherwise scan for calls marked in_transaction.  */
  	regions = ipa_tm_region_init (node);
  	if (regions)
  	  {
--- 4490,4496 ----
  	    continue;
  	  }
  
! 	/* ... otherwise scan for calls that are in a transaction.  */
  	regions = ipa_tm_region_init (node);
  	if (regions)
  	  {
*************** ipa_tm_execute (void)
*** 4629,4634 ****
--- 4647,4653 ----
    VEC_free (cgraph_node_p, heap, tm_callees);
    VEC_free (cgraph_node_p, heap, worklist);
    bitmap_obstack_release (&tm_obstack);
+   BITMAP_FREE (bb_in_TM_region);
  
    for (node = cgraph_nodes; node; node = node->next)
      node->aux = NULL;

Re: [trans-mem] PR 46567

[Richard Henderson]

On 01/21/2011 08:15 AM, Andrew MacLeod wrote:
> If it seems reasonable I'll add the testcase into the testsuite and  the patch as well.

It's a pretty good approach.  A couple of things I'd change below...

> I guess I could also remove  gimple_call_in_transaction_p and references as well eh?

Yes please.

>   get_tm_region_blocks (basic_block entry_block,
>   		      bitmap exit_blocks,
>   		      bitmap irr_blocks,
> + 		      bitmap all_region_blocks,
>   		      bool stop_at_irrevocable_p)
>   {
>     VEC(basic_block, heap) *bbs = NULL;
> *************** get_tm_region_blocks (basic_block entry_
> *** 2354,2359 ****
> --- 2353,2361 ----
>       }
>     while (i < VEC_length (basic_block, bbs));
>   
> +   if (all_region_blocks)
> +     bitmap_ior_into (all_region_blocks, visited_blocks);
> + 

You wouldn't need the IOR if we make the parameter visited_blocks, and if
it's null allocate the thing.  That should work since TM regions do not
overlap.  Although perhaps that makes the interface a bit confusing...

>     bitmap_obstack_initialize (&tm_obstack);
> +   bb_in_TM_region = BITMAP_ALLOC (NULL);

  BITMAP_ALLOC (&tm_obstack).

and then you don't need the free at the end, since we junk the entire
obstack all at once.


r~

Re: [trans-mem] PR 46567

[Aldy Hernandez]

> You wouldn't need the IOR if we make the parameter visited_blocks, and if
> it's null allocate the thing.  That should work since TM regions do not
> overlap.  Although perhaps that makes the interface a bit confusing...

More confusing?  Please no, then :).

Do put comments, cause I'm liable to forget pretty s...err, what where 
we talking about?

Thanks Andrew.

Aldy

Re: [trans-mem] PR 46567

[Andrew MacLeod]

On 01/21/2011 12:59 PM, Richard Henderson wrote:
>
> You wouldn't need the IOR if we make the parameter visited_blocks, and if
> it's null allocate the thing.  That should work since TM regions do not
> overlap.  Although perhaps that makes the interface a bit confusing...
>

Thats exactly the way I had it originally, and decided this was cleaner 
unless it became a performance issue. I wasn't sure that someone might 
not be making a call in someday on a sub region or something else for 
some reason and not get their expected result. it sounded difficult to 
find.  I dont think IORs are very expensive.
>>      bitmap_obstack_initialize (&tm_obstack);
>> +   bb_in_TM_region = BITMAP_ALLOC (NULL);
>    BITMAP_ALLOC (&tm_obstack).
>
> and then you don't need the free at the end, since we junk the entire
> obstack all at once.
>

/me is always suspicious of obstacks, but OK  :-)

Andrew

Re: [trans-mem] PR 46567

[Andrew MacLeod]

On 01/21/2011 01:47 PM, Richard Henderson wrote:
>
>> Thats exactly the way I had it originally, and decided this was cleaner unless it became a performance issue. I wasn't sure that someone might not be making a call in someday on a sub region or something else for some reason and not get their expected result. it sounded difficult to find.  I dont think IORs are very expensive.
> Fair enough.  ;-)
>
>

So, I can go all the way to removing the flag GF_CALL_IN_TRANSACTION 
from gimple.h now.  What's procedure?  Do I collapse the values of the 
values which come after( ie, make them 5 and 6 instead of 6 and 7), or 
just leave the shift of 5 blank like I was planning?
     GF_CALL_VA_ARG_PACK         = 1 << 4,
     GF_CALL_NOTHROW             = 1 << 6,
     GF_CALL_NOINLINE            = 1 << 7,

Andrew

Re: [trans-mem] PR 46567

[Richard Henderson]

On 01/21/2011 10:44 AM, Andrew MacLeod wrote:
> On 01/21/2011 12:59 PM, Richard Henderson wrote:
>>
>> You wouldn't need the IOR if we make the parameter visited_blocks, and if
>> it's null allocate the thing.  That should work since TM regions do not
>> overlap.  Although perhaps that makes the interface a bit confusing...
>>
> 
> Thats exactly the way I had it originally, and decided this was cleaner unless it became a performance issue. I wasn't sure that someone might not be making a call in someday on a sub region or something else for some reason and not get their expected result. it sounded difficult to find.  I dont think IORs are very expensive.

Fair enough.  ;-)


r~

Re: [trans-mem] PR 46567

[Richard Henderson]

On 01/21/2011 10:52 AM, Andrew MacLeod wrote:
> On 01/21/2011 01:47 PM, Richard Henderson wrote:
>>
>>> Thats exactly the way I had it originally, and decided this was cleaner unless it became a performance issue. I wasn't sure that someone might not be making a call in someday on a sub region or something else for some reason and not get their expected result. it sounded difficult to find.  I dont think IORs are very expensive.
>> Fair enough.  ;-)
>>
>>
> 
> So, I can go all the way to removing the flag GF_CALL_IN_TRANSACTION from gimple.h now.  What's procedure?  Do I collapse the values of the values which come after( ie, make them 5 and 6 instead of 6 and 7), or just leave the shift of 5 blank like I was planning?
>     GF_CALL_VA_ARG_PACK         = 1 << 4,
>     GF_CALL_NOTHROW             = 1 << 6,
>     GF_CALL_NOINLINE            = 1 << 7,

Given that mainline has

    GF_CALL_VA_ARG_PACK         = 1 << 4,
    GF_CALL_NOTHROW             = 1 << 5,

It might be better if you collapse that number so as to make an eventual
merge from mainline just a teeny bit easier.


r~

Re: [trans-mem] PR 46567

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

On 01/21/2011 02:00 PM, Richard Henderson wrote:
>
> It might be better if you collapse that number so as to make an eventual
> merge from mainline just a teeny bit easier.
How this look then?

seems to bootstrap and pass testcases on x86_64-unknown-linux-gnu.

Andrew

[-- Attachment #2: pr46567 --]
[-- Type: text/plain, Size: 10963 bytes --]

2011-01-21  Andrew MacLeod  <amacleod@redhat.com>

	PR/46567 
	* gimple-pretty-print (dump_gimple_call): Don't check in_transaction.
	* trans-mem.c (examine_call_tm): Remove gimple_call_set_in_transaction.
	(get_tm_region_blocks): Add new parameter for returning all blocks in 
	transactions.
	(execute_tm_mark, expand_regions_1, execute_tm_memopt): Add extra
	parameter to call.
	(bb_in_TM_region): New bitfield for BB's in tranactions.
	(ipa_tm_scan_calls_transaction): Check new bitfield.
	(ipa_tm_note_irrevocable): Check new bitfield.
	(ipa_tm_propagate_irr, ipa_tm_diagnose_transaction): Add extra param.
	(ipa_tm_insert_gettmclone_call): Remove set_transaction hack.
	(ipa_tm_execute): Build bb bitmap.
	* gimple.h (enum gf_mask): Remove GF_CALL_IN_TRANSACTION.
	(gimple_call_set_in_transaction, gimple_call_in_transaction_p): Delete.

	* testsuite/g++.dg/tm/pr46567.C: New.

Index: gimple-pretty-print.c
===================================================================
*** gimple-pretty-print.c	(revision 168633)
--- gimple-pretty-print.c	(working copy)
*************** dump_gimple_call (pretty_printer *buffer
*** 535,542 ****
      pp_string (buffer, " [return slot optimization]");
    if (gimple_call_tail_p (gs))
      pp_string (buffer, " [tail call]");
-   if (gimple_call_in_transaction_p (gs))
-     pp_string (buffer, " [in atomic]");
  
    /* Dump the arguments of _ITM_beginTransaction sanely.  */
    if (TREE_CODE (fn) == ADDR_EXPR)
--- 535,540 ----
Index: trans-mem.c
===================================================================
*** trans-mem.c	(revision 168633)
--- trans-mem.c	(working copy)
*************** examine_call_tm (unsigned *state, gimple
*** 1515,1522 ****
    gimple stmt = gsi_stmt (*gsi);
    tree fn;
  
-   gimple_call_set_in_transaction (stmt, true);
- 
    if (is_tm_pure_call (stmt))
      return;
  
--- 1515,1520 ----
*************** static VEC (basic_block, heap) *
*** 2320,2325 ****
--- 2318,2324 ----
  get_tm_region_blocks (basic_block entry_block,
  		      bitmap exit_blocks,
  		      bitmap irr_blocks,
+ 		      bitmap all_region_blocks,
  		      bool stop_at_irrevocable_p)
  {
    VEC(basic_block, heap) *bbs = NULL;
*************** get_tm_region_blocks (basic_block entry_
*** 2354,2359 ****
--- 2353,2361 ----
      }
    while (i < VEC_length (basic_block, bbs));
  
+   if (all_region_blocks)
+     bitmap_ior_into (all_region_blocks, visited_blocks);
+ 
    BITMAP_FREE (visited_blocks);
    return bbs;
  }
*************** execute_tm_mark (void)
*** 2395,2400 ****
--- 2397,2403 ----
        queue = get_tm_region_blocks (region->entry_block,
  				    region->exit_blocks,
  				    region->irr_blocks,
+ 				    NULL,
  				    /*stop_at_irr_p=*/true);
        for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i)
  	expand_block_tm (region, bb);
*************** expand_regions_1 (struct tm_region *regi
*** 2629,2634 ****
--- 2632,2638 ----
        queue = get_tm_region_blocks (region->entry_block,
  				    region->exit_blocks,
  				    region->irr_blocks,
+ 				    NULL,
  				    /*stop_at_irr_p=*/false);
        expand_transaction (region);
        for (i = 0; VEC_iterate (basic_block, queue, i, bb); ++i)
*************** execute_tm_memopt (void)
*** 3299,3305 ****
        /* Save all BBs for the current region.  */
        bbs = get_tm_region_blocks (region->entry_block,
  				  region->exit_blocks,
! 				  region->irr_blocks, false);
  
        /* Collect all the memory operations.  */
        for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
--- 3303,3311 ----
        /* Save all BBs for the current region.  */
        bbs = get_tm_region_blocks (region->entry_block,
  				  region->exit_blocks,
! 				  region->irr_blocks, 
! 				  NULL, 
! 				  false);
  
        /* Collect all the memory operations.  */
        for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
*************** struct gimple_opt_pass pass_tm_memopt =
*** 3363,3369 ****
  	(a) For all local public functions marked tm_callable, push
  	    it onto the tm_callee queue.
  
! 	(b) For all local functions, scan for calls marked in_transaction.
  	    Push the caller and callee onto the tm_caller and tm_callee
  	    queues.  Count the number of callers for each callee.
  
--- 3369,3375 ----
  	(a) For all local public functions marked tm_callable, push
  	    it onto the tm_callee queue.
  
! 	(b) For all local functions, scan for calls in transaction blocks.
  	    Push the caller and callee onto the tm_caller and tm_callee
  	    queues.  Count the number of callers for each callee.
  
*************** DEF_VEC_ALLOC_P (cgraph_node_p, heap);
*** 3437,3442 ****
--- 3443,3450 ----
  
  typedef VEC (cgraph_node_p, heap) *cgraph_node_queue;
  
+ /* List of all BB's that are in transactional regions.  */
+ static bitmap bb_in_TM_region = NULL;
  
  /* Return the ipa data associated with NODE, allocating zeroed memory
     if necessary.  */
*************** ipa_tm_scan_calls_transaction (struct cg
*** 3482,3488 ****
    tree replacement;
  
    for (e = node->callees; e ; e = e->next_callee)
!     if (gimple_call_in_transaction_p (e->call_stmt))
        {
  	struct tm_ipa_cg_data *d;
  
--- 3490,3497 ----
    tree replacement;
  
    for (e = node->callees; e ; e = e->next_callee)
!     /* Check if the callee is in a transactional region. */ 
!     if (bitmap_bit_p (bb_in_TM_region, gimple_bb (e->call_stmt)->index))
        {
  	struct tm_ipa_cg_data *d;
  
*************** ipa_tm_note_irrevocable (struct cgraph_n
*** 3547,3553 ****
  	 above all.  */
        if (is_tm_safe_or_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);
      }
--- 3556,3565 ----
  	 above all.  */
        if (is_tm_safe_or_pure (e->caller->decl))
  	continue;
! 
!       gcc_assert (gimple_bb (e->call_stmt) != NULL);
!       /* Check if the callee is in a transactional region. */ 
!       if (bitmap_bit_p (bb_in_TM_region, gimple_bb (e->call_stmt)->index))
  	d->want_irr_scan_normal = true;
        maybe_push_queue (e->caller, worklist_p, &d->in_worklist);
      }
*************** ipa_tm_propagate_irr (basic_block entry_
*** 3670,3676 ****
    if (old_irr && bitmap_bit_p (old_irr, entry_block->index))
      return;
  
!   bbs = get_tm_region_blocks (entry_block, exit_blocks, NULL, false);
    do
      {
        basic_block bb = VEC_pop (basic_block, bbs);
--- 3682,3688 ----
    if (old_irr && bitmap_bit_p (old_irr, entry_block->index))
      return;
  
!   bbs = get_tm_region_blocks (entry_block, exit_blocks, NULL, NULL, false);
    do
      {
        basic_block bb = VEC_pop (basic_block, bbs);
*************** ipa_tm_diagnose_transaction (struct cgra
*** 3921,3927 ****
  	size_t i;
  
  	bbs = get_tm_region_blocks (r->entry_block, r->exit_blocks,
! 				    r->irr_blocks, false);
  
  	for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
  	  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
--- 3933,3939 ----
  	size_t i;
  
  	bbs = get_tm_region_blocks (r->entry_block, r->exit_blocks,
! 				    r->irr_blocks, NULL, false);
  
  	for (i = 0; VEC_iterate (basic_block, bbs, i, bb); ++i)
  	  for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
*************** ipa_tm_insert_gettmclone_call (struct cg
*** 4184,4198 ****
    ret = make_ssa_name (ret, g);
    gimple_call_set_lhs (g, ret);
  
-   /* ??? If we need to go irrevocable, we can fail the intermediate
-      commit and restart the transaction.  But representing that means
-      splitting this basic block, which means busting all of the bitmaps
-      we've put together, as well as the dominator tree.  Perhaps we
-      can get away with ignoring it, since the indirect function that
-      we're about to call should also have the back edge.  */
-   if (0 && !safe)
-     gimple_call_set_in_transaction (g, true);
- 
    gsi_insert_before (gsi, g, GSI_SAME_STMT);
  
    cgraph_create_edge (node, cgraph_node (gettm_fn), g, 0,
--- 4196,4201 ----
*************** ipa_tm_execute (void)
*** 4445,4450 ****
--- 4448,4468 ----
  #endif
  
    bitmap_obstack_initialize (&tm_obstack);
+   bb_in_TM_region = BITMAP_ALLOC (&tm_obstack);
+ 
+   /* Build a bitmap of all BB"s inside transaction regions.  */
+   for (node = cgraph_nodes; node; node = node->next)
+     if (node->reachable && node->lowered
+ 	&& cgraph_function_body_availability (node) >= AVAIL_OVERWRITABLE)
+       {
+ 	struct tm_region *regions;
+ 	regions = ipa_tm_region_init (node); 
+ 	for ( ; regions; regions = regions->next)
+ 	  {
+ 	    get_tm_region_blocks (regions->entry_block, NULL, NULL, 
+ 				  bb_in_TM_region, false);
+ 	  }
+       }
  
    /* For all local functions marked tm_callable, queue them.  */
    for (node = cgraph_nodes; node; node = node->next)
*************** ipa_tm_execute (void)
*** 4472,4478 ****
  	    continue;
  	  }
  
! 	/* ... otherwise scan for calls marked in_transaction.  */
  	regions = ipa_tm_region_init (node);
  	if (regions)
  	  {
--- 4490,4496 ----
  	    continue;
  	  }
  
! 	/* ... otherwise scan for calls that are in a transaction.  */
  	regions = ipa_tm_region_init (node);
  	if (regions)
  	  {
Index: gimple.h
===================================================================
*** gimple.h	(revision 168633)
--- gimple.h	(working copy)
*************** enum gf_mask {
*** 106,114 ****
      GF_CALL_RETURN_SLOT_OPT	= 1 << 2,
      GF_CALL_TAILCALL		= 1 << 3,
      GF_CALL_VA_ARG_PACK		= 1 << 4,
!     GF_CALL_IN_TRANSACTION	= 1 << 5,
!     GF_CALL_NOTHROW		= 1 << 6,
!     GF_CALL_NOINLINE		= 1 << 7,
      GF_OMP_PARALLEL_COMBINED	= 1 << 0,
  
      /* True on an GIMPLE_OMP_RETURN statement if the return does not require
--- 106,113 ----
      GF_CALL_RETURN_SLOT_OPT	= 1 << 2,
      GF_CALL_TAILCALL		= 1 << 3,
      GF_CALL_VA_ARG_PACK		= 1 << 4,
!     GF_CALL_NOTHROW		= 1 << 5,
!     GF_CALL_NOINLINE		= 1 << 6,
      GF_OMP_PARALLEL_COMBINED	= 1 << 0,
  
      /* True on an GIMPLE_OMP_RETURN statement if the return does not require
*************** gimple_call_va_arg_pack_p (gimple s)
*** 2249,2279 ****
  }
  
  
- /* If IN_TRANSACTION_P is true, GIMPLE_CALL S is within the dynamic scope of
-    a GIMPLE_TRANSACTION transaction.  */
- 
- static inline void
- gimple_call_set_in_transaction (gimple s, bool in_transaction_p)
- {
-   GIMPLE_CHECK (s, GIMPLE_CALL);
-   if (in_transaction_p)
-     s->gsbase.subcode |= GF_CALL_IN_TRANSACTION;
-   else
-     s->gsbase.subcode &= ~GF_CALL_IN_TRANSACTION;
- }
-   
- 
- /* Return true if GIMPLE_CALL S is within the dynamic scope of
-    a transaction.  */
- 
- static inline bool
- gimple_call_in_transaction_p (gimple s)
- {
-   GIMPLE_CHECK (s, GIMPLE_CALL);
-   return (s->gsbase.subcode & GF_CALL_IN_TRANSACTION) != 0;
- }
- 
- 
  /* Return true if S is a noreturn call.  */
  
  static inline bool
--- 2248,2253 ----

Re: [trans-mem] PR 46567

[Richard Henderson]

On 01/21/2011 12:47 PM, Andrew MacLeod wrote:
> 	PR/46567 
> 	* gimple-pretty-print (dump_gimple_call): Don't check in_transaction.
> 	* trans-mem.c (examine_call_tm): Remove gimple_call_set_in_transaction.
> 	(get_tm_region_blocks): Add new parameter for returning all blocks in 
> 	transactions.
> 	(execute_tm_mark, expand_regions_1, execute_tm_memopt): Add extra
> 	parameter to call.
> 	(bb_in_TM_region): New bitfield for BB's in tranactions.
> 	(ipa_tm_scan_calls_transaction): Check new bitfield.
> 	(ipa_tm_note_irrevocable): Check new bitfield.
> 	(ipa_tm_propagate_irr, ipa_tm_diagnose_transaction): Add extra param.
> 	(ipa_tm_insert_gettmclone_call): Remove set_transaction hack.
> 	(ipa_tm_execute): Build bb bitmap.
> 	* gimple.h (enum gf_mask): Remove GF_CALL_IN_TRANSACTION.
> 	(gimple_call_set_in_transaction, gimple_call_in_transaction_p): Delete.
> 
> 	* testsuite/g++.dg/tm/pr46567.C: New.

Ok.


r~

Re: [trans-mem] PR 46567

[Patrick Marlier]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

Hi Andrew,

Attached a reduced test case that raise the same error in 
ipa_tm_decrement_clone_counts() with O0.
I tested with or without your patch so I am not sure this is related 
with the previous bug.

$ gcc -fgnu-tm -O0 -S pr46567-2.i -o testcase.s
pr46567-2.i: In function ‘readLoop’:
pr46567-2.i:18:1: internal compiler error: in 
ipa_tm_decrement_clone_counts, at trans-mem.c:3748
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

Thanks.

Patrick

PS: I have also added the test case to the bugzilla.


On 01/21/2011 09:47 PM, Andrew MacLeod wrote:
> On 01/21/2011 02:00 PM, Richard Henderson wrote:
>>
>> It might be better if you collapse that number so as to make an eventual
>> merge from mainline just a teeny bit easier.
> How this look then?
>
> seems to bootstrap and pass testcases on x86_64-unknown-linux-gnu.
>
> Andrew

[-- Attachment #2: pr46567-2.i --]
[-- Type: text/plain, Size: 305 bytes --]

__attribute__((transaction_callable)) 
static void SeqfileGetLine ()
{
  SSIGetFilePosition ();
}

__attribute__((transaction_callable))
static void readLoop (int addfirst)
{
  if (!addfirst)
    {
      if (!addfirst)
        {
          SSIGetFilePosition ();
        }
      SeqfileGetLine ();
    }
}

Re: [trans-mem] PR 46567

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

On 01/28/2011 09:18 AM, Patrick Marlier wrote:
> Hi Andrew,
>
> Attached a reduced test case that raise the same error in 
> ipa_tm_decrement_clone_counts() with O0.
> I tested with or without your patch so I am not sure this is related 
> with the previous bug.

Give this patch a try. I was narrowing down on this with the large 
testcase, this smaller one just accelerated it a bit :-)

When a block is processed as irrevocable and added to the 
irrevocable_block_clone list, problems were occuring when that same 
block was called from within another region which also contained it.  Or 
something to that effect.   What was happening was ipa_tm_propagate_irr 
was proceeding to add a whole pile of blocks to new_irr to be processed, 
but didn't stop to check if some of those blocks had already been 
processed.

in the case of your small testcase,  SSIGetFilePosition was pocessed in 
readloop, and then when  SeqfileGetLine was processed, 
SSIGetFilePosition was a subblock which just simply blindly had its bit 
set, and it was then processed again, and the clone counter decremented, 
and boom.

So try the following patch.  Does this seem reasonable richard?  It 
passes the small testcase as well as the original larger one, and no new 
failures in the testsuite.

Andrew


[-- Attachment #2: tm2.patch --]
[-- Type: text/plain, Size: 1494 bytes --]

2011-01-28	Andrew MacLeod  <amacleod@redhat.com>

	* trans-mem.c (ipa_tm_propagate_irr): Don't reprocess blocks already in
	the old irrevocable list.


Index: trans-mem.c
===================================================================
*** trans-mem.c	(revision 169369)
--- trans-mem.c	(working copy)
*************** ipa_tm_propagate_irr (basic_block entry_
*** 3702,3709 ****
  	      }
  	  if (all_son_irr)
  	    {
! 	      bitmap_set_bit (new_irr, bb->index);
! 	      this_irr = true;
  	    }
  	}
  
--- 3702,3713 ----
  	      }
  	  if (all_son_irr)
  	    {
! 	      /* Add block to new_irr if it hasn't already been processed. */
! 	      if (!old_irr || !bitmap_bit_p (old_irr, bb->index))
! 	        {
! 		  bitmap_set_bit (new_irr, bb->index);
! 		  this_irr = true;
! 		}
  	    }
  	}
  
*************** ipa_tm_propagate_irr (basic_block entry_
*** 3714,3720 ****
  	  for (son = first_dom_son (CDI_DOMINATORS, bb);
  	       son;
  	       son = next_dom_son (CDI_DOMINATORS, son))
! 	    bitmap_set_bit (new_irr, son->index);
  	}
      }
    while (!VEC_empty (basic_block, bbs));
--- 3718,3728 ----
  	  for (son = first_dom_son (CDI_DOMINATORS, bb);
  	       son;
  	       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);
! 	    }
  	}
      }
    while (!VEC_empty (basic_block, bbs));

Re: [trans-mem] PR 46567

[Patrick Marlier]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

Hi Andrew,

Here a reduced test case that raises this ICE with current svn:
gcc -fgnu-tm -O0 ICE_ipa_tm_propagate_irr.i
ICE_ipa_tm_propagate_irr.i: In function ‘readLoop’:
ICE_ipa_tm_propagate_irr.i:22:1: internal compiler error: in 
ipa_tm_decrement_clone_counts, at trans-mem.c:3748
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

and raises this ICE with your latest patch (tm2.patch)
gcc -fgnu-tm -O0 ICE_ipa_tm_propagate_irr.i
ICE_ipa_tm_propagate_irr.i: In function ‘readLoop’:
ICE_ipa_tm_propagate_irr.i:22:1: internal compiler error: in 
ipa_tm_propagate_irr, at trans-mem.c:3723
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

Thanks in advance,
Have a nice day,

Patrick.



On 01/28/2011 10:15 PM, Andrew MacLeod wrote:
> On 01/28/2011 09:18 AM, Patrick Marlier wrote:
>> Hi Andrew,
>>
>> Attached a reduced test case that raise the same error in
>> ipa_tm_decrement_clone_counts() with O0.
>> I tested with or without your patch so I am not sure this is related
>> with the previous bug.
>
> Give this patch a try. I was narrowing down on this with the large
> testcase, this smaller one just accelerated it a bit :-)
>
> When a block is processed as irrevocable and added to the
> irrevocable_block_clone list, problems were occuring when that same
> block was called from within another region which also contained it.  Or
> something to that effect.   What was happening was ipa_tm_propagate_irr
> was proceeding to add a whole pile of blocks to new_irr to be processed,
> but didn't stop to check if some of those blocks had already been
> processed.
>
> in the case of your small testcase,  SSIGetFilePosition was pocessed in
> readloop, and then when  SeqfileGetLine was processed,
> SSIGetFilePosition was a subblock which just simply blindly had its bit
> set, and it was then processed again, and the clone counter decremented,
> and boom.
>
> So try the following patch.  Does this seem reasonable richard?  It
> passes the small testcase as well as the original larger one, and no new
> failures in the testsuite.
>
> Andrew
>

[-- Attachment #2: ICE_ipa_tm_propagate_irr.i --]
[-- Type: text/plain, Size: 354 bytes --]

struct ReadSeqVars { };
int rms_feof();

__attribute__((transaction_callable)) static void SeqfileGetLine()
{
	if (SSIGetFilePosition())
	{
	}
}

__attribute__((transaction_callable)) static void readLoop(struct ReadSeqVars *V)
{
	SeqfileGetLine();
	if (V == (void *)0 && rms_feof())
	{
	}

	int pos;
	for (pos = 0; pos < 1; pos++)
        {
        }
}