Re: [Bug c++/46269] [trans-mem] internal compiler error in expand_block_tm of trans-mem.c

[Richard Henderson <rth at redhat dot com>]

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,
the call to BUILT_IN_TM_IRREVOCABLE that we insertted before
the ASM does that.

This function ought to be collecting what IS, as distinguished
from the ipa pass which ought to collect what SHOULD BE.

> +      /* A clone may have irrevocable blocks (i.e. GIMPLE_ASM) even if
> +	 we're not technically inside a transaction.  */

The comment is badly worded.  A clone by definition means we ARE
inside of a transaction, just one that starts somewhere up the
dynamic call chain.

> +/* Add NODE to the end of QUEUE, unless IN_QUEUE_P indicates that it
> +   is already present.  Also adds NODE to the end of QUEUE2, but only
> +   if QUEUE2 is non-NULL.  */
>  
>  static void
>  maybe_push_queue (struct cgraph_node *node,
> -		  cgraph_node_queue *queue_p, bool *in_queue_p)
> +		  cgraph_node_queue *queue_p,
> +		  cgraph_node_queue *queue2_p,
> +		  bool *in_queue_p)

Err, really?  Manipulating two queues at once?  Why?

This seems wrong, since there ought to be two in_queue_p variables
involved, at which point what's wrong with two calls to this function?

> @@ -3659,6 +3682,10 @@ ipa_tm_scan_irr_function (struct cgraph_
>    VEC (basic_block, heap) *queue;
>    bool ret = false;
>  
> +  /* Skip things like new/delete operators.  */
> +  if (!DECL_STRUCT_FUNCTION (node->decl))
> +    return false;

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

> +  worklist = VEC_copy (cgraph_node_p, heap, tm_callees);

Don't you have to set in_worklist for each of these?


r~