Patchwork 19/n: trans-mem: middle end/misc patches (LAST PATCH)

login
register
mail settings
Submitter Aldy Hernandez
Date Nov. 6, 2011, 6:53 p.m.
Message ID <4EB6D797.4070309@redhat.com>
Download mbox | patch
Permalink /patch/123957/
State New
Headers show

Comments

Aldy Hernandez - Nov. 6, 2011, 6:53 p.m.
[rth, more comments for you below]

On 11/04/11 04:14, Richard Guenther wrote:

>>     new_version = cgraph_create_node (new_decl);
>>
>> -   new_version->analyzed = true;
>> +   new_version->analyzed = old_version->analyzed;
>
> Hm?  analyzed means "with body", sure you have a body if you clone.
>
>>     new_version->local = old_version->local;
>>     new_version->local.externally_visible = false;
>>     new_version->local.local = true;
>> @@ -2294,6 +2294,7 @@ cgraph_copy_node_for_versioning (struct
>>     new_version->rtl = old_version->rtl;
>>     new_version->reachable = true;
>>     new_version->count = old_version->count;
>> +   new_version->lowered = true;
>
> OTOH this isn't necessary true.  cgraph exists before lowering.

I don't understand what you want me to do on either of these two 
comments.  Could you elaborate?

>> +  /* TM pure functions should not get inlined if the outer function is
>> +     a TM safe function.  */
>> +  else if (flag_tm
>
> Please move flag checks into the respective prediates.  Any reason
> why the is_tm_pure () predicate wouldn't already do the correct thing
> with !flag_tm?

Done.

>> +  /* Map gimple stmt to tree label (or list of labels) for transaction
>> +     restart and abort.  */
>> +  htab_t GTY ((param_is (struct tm_restart_node))) tm_restart;
>> +
>
> As this maps 'gimple' to tree shouldn't this go to fn->gimple_df instead?
> That way you avoid growing generic struct function.  Or in to eh_status,
> if that looks like a better fit.

Done.

>> +  /* Mark all calls that can have a transaction restart.  */
>
> Why isn't this done when we expand the call?  This walking of the
> RTL sequence looks like a hack (an easy one, albeit).
>
>> +  if (cfun->tm_restart&&  is_gimple_call (stmt))
>> +    {
>> +      struct tm_restart_node dummy;
>> +      void **slot;
>> +
>> +      dummy.stmt = stmt;
>> +      slot = htab_find_slot (cfun->tm_restart,&dummy, NO_INSERT);
>> +      if (slot)
>> +       {
>> +         struct tm_restart_node *n = (struct tm_restart_node *) *slot;
>> +         tree list = n->label_or_list;
>> +         rtx insn;
>> +
>> +         for (insn = next_real_insn (last); !CALL_P (insn);
>> +              insn = next_real_insn (insn))
>> +           continue;
>> +
>> +         if (TREE_CODE (list) == LABEL_DECL)
>> +           add_reg_note (insn, REG_TM, label_rtx (list));
>> +         else
>> +           for (; list ; list = TREE_CHAIN (list))
>> +             add_reg_note (insn, REG_TM, label_rtx (TREE_VALUE (list)));
>> +       }
>> +    }

I can certainly move this to expand_call_stmt() if you prefer.  Do you 
have an objection to the RTL walk?  This isn't my code, but I'm open to 
suggestions on an alternative to implement.

>> +  /* After expanding, the tm_restart map is no longer needed.  */
>> +  cfun->tm_restart = NULL;
>
> You should still free it, to not confuse the statistics code I think.

Done.

>> +finish_tm_clone_pairs (void)
>> +{
>> +  bool switched = false;
>> +
>> +  if (tm_clone_pairs == NULL)
>> +    return;
>> +
>> +  htab_traverse_noresize (tm_clone_pairs, finish_tm_clone_pairs_1,
>> +                         (void *)&switched);
>
> This makes the generated table dependent on memory layout.  You
> need to walk the pairs in some deterministic order.  In fact why not
> walk all cgraph_nodes looking for the pairs - they should be still
> in the list of clones for a node and you've marked it with DECL_TM_CLONE.
> You can then sort them by cgraph node uid.
>
> Did you check bootstrapping GCC with TM enabled and address-space
> randomization turned on?

Actually, the table organization is irrelevant, because upon registering 
of the table in the runtime, we qsort the entire thing.  We then do a 
binary search to find items.  See _ITM_registerTMCloneTable() and 
find_clone() in the libitm runtime.

>> +/* In gtm-low.c  */
>> +extern bool is_transactional_stmt (const_gimple);
>> +
>
> gimple.h please.  looks like a gimple predicate as well, so the implementation
> should be in gimple.c?

Woo hoo!  Unused function.  I've removed it altogether.

>>         case GIMPLE_GOTO:
>> -          if (!computed_goto_p (stmt))
>> +         if (!computed_goto_p (stmt))
>>             {
>> -             tree new_dest = main_block_label (gimple_goto_dest (stmt));
>> -             gimple_goto_set_dest (stmt, new_dest);
>> +             label = gimple_goto_dest (stmt);
>> +             new_label = main_block_label (label);
>> +             if (new_label != label)
>> +               gimple_goto_set_dest (stmt, new_label);
>
> What's the reason for this changes?  Optimization?

Yes.  Rth can elaborate if you deem necessary.

>> +/* Verify the contents of a GIMPLE_TRANSACTION.  Returns true if there
>> +   is a problem, otherwise false.  */
>> +
>> +static bool
>> +verify_gimple_transaction (gimple stmt)
>> +{
>> +  tree lab = gimple_transaction_label (stmt);
>> +  if (lab != NULL&&  TREE_CODE (lab) != LABEL_DECL)
>> +    return true;
>
> ISTR this has substatements, so you should handle this in
> verify_gimple_in_seq_2 and make sure to verify those substatements.

I have added verification for the substatements in 
verify_gimple_transaction()...

>> @@ -4155,6 +4210,9 @@ verify_gimple_stmt (gimple stmt)
>>      case GIMPLE_ASM:
>>        return false;
>>
>> +    case GIMPLE_TRANSACTION:
>> +      return verify_gimple_transaction (stmt);
>> +
>
> Not here.

...but we still need to check the transaction in verify_gimple_stmt() 
(as well as in verify_gimple_in_seq_2), because verify_gimple_in_cfg() 
will verify a gimple_transaction by calling verify_gimple_stmt, so we 
must handle GIMPLE_TRANSACTION in verify_gimple_stmt also.

>> +       case GIMPLE_TRANSACTION:
>> +         err |= verify_gimple_in_seq_2 (gimple_transaction_body (stmt));
>> +         break;

I am calling verify_gimple_transaction() now.  See patch.

>> +    case GIMPLE_TRANSACTION:
>> +      /* The ABORT edge has a stored label associated with it, otherwise
>> +        the edges are simply redirectable.  */
>> +      /* ??? We don't really need this label after the cfg is created.  */
>> +      if (e->flags == 0)
>> +       gimple_transaction_set_label (stmt, gimple_block_label (dest));
>
> So why set it (and thus keep it live)?

This seems like leftovers from a previous incantation.  However, I'm not 
100% sure, so I have disabled the code, but left it in a comment.  A 
full bootstrap/regtest revealed no regressions.

rth, do you have any objections to remove this?

Tested on x86-64 Linux.

OK for branch?

p.s. This changelog entry is for ChangeLog.tm, but I will adapt a merged 
ChangeLog entry for ChangeLog.tm-merge as well.  And will do so from now on.
* tree-cfg.c (verify_gimple_transaction): Verify body.  Move down.
	(verify_gimple_in_seq_2): Verify the label in a
	GIMPLE_TRANSACTION.
	* function.h (struct function): Move tm_restart field to struct
	gimple_df in tree-flow.h
	Move tm_restart_node to tree-flow.h
	* tree-flow.h (struct gimple_df): New location for tm_restart
	field.
	New location for tm_restart_node.
	(is_transactional_stmt): Remove.
	* trans-mem.c (is_transactional_stmt): Remove.
	(make_tm_edge): Field tm_restart is now in gimple_df.
	* cfgexpand.c (gimple_expand_cfg): Field tm_restart is now in
	cfun->gimple_df.
	Free tm_restart.
	* cfgexpand.c (expand_gimple_stmt): Field tm_restart is now in
	gimple_df.
	* ipa-inline.c (can_inline_edge_p): Do not check flag_tm.
	* trans-mem.c (is_tm_pure): Check flag_tm.
	(is_tm_safe): Same.
Richard Henderson - Nov. 6, 2011, 8:20 p.m.
On 11/06/2011 10:53 AM, Aldy Hernandez wrote:
>> Did you check bootstrapping GCC with TM enabled and address-space
>> randomization turned on?
> 
> Actually, the table organization is irrelevant, because upon
> registering of the table in the runtime, we qsort the entire thing.

False.  You get the equivalent of bootstrap comparison mismatches.
If we actually used tm during the bootstrap.

The simplest thing to do is to change the hash this table uses.
E.g. use the DECL_UID right from the start, rather than the pointer.

>>> -          if (!computed_goto_p (stmt))
>>> +         if (!computed_goto_p (stmt))
>>>             {
>>> -             tree new_dest = main_block_label (gimple_goto_dest (stmt));
>>> -             gimple_goto_set_dest (stmt, new_dest);
>>> +             label = gimple_goto_dest (stmt);
>>> +             new_label = main_block_label (label);
>>> +             if (new_label != label)
>>> +               gimple_goto_set_dest (stmt, new_label);
>>
>> What's the reason for this changes?  Optimization?
> 
> Yes.  Rth can elaborate if you deem necessary.

Really?  I have no idea what this change achieves.
I actually wonder if this is a merge error.

>>> +    case GIMPLE_TRANSACTION:
>>> +      /* The ABORT edge has a stored label associated with it, otherwise
>>> +        the edges are simply redirectable.  */
>>> +      /* ??? We don't really need this label after the cfg is created.  */
>>> +      if (e->flags == 0)
>>> +       gimple_transaction_set_label (stmt, gimple_block_label (dest));
>>
>> So why set it (and thus keep it live)?
> 
> This seems like leftovers from a previous incantation.  However, I'm not 100% sure, so I have disabled the code, but left it in a comment.  A full bootstrap/regtest revealed no regressions.
> 
> rth, do you have any objections to remove this?

I think that the comment is wrong.  We need that edge, and the label updated until pass_tm_edges, at which point the GIMPLE_TRANSACTION itself goes away.  Thus that label is live throughout the live of the GIMPLE_TRANSACTION node.

Delete that ??? comment instead.

Patch is otherwise ok.


r~
Richard Henderson - Nov. 6, 2011, 9:18 p.m.
> On 11/04/11 04:14, Richard Guenther wrote:
>>>     new_version = cgraph_create_node (new_decl);
>>>
>>> -   new_version->analyzed = true;
>>> +   new_version->analyzed = old_version->analyzed;
>>
>> Hm?  analyzed means "with body", sure you have a body if you clone.

Incidentally, for TM we also clone functions that do NOT have a body.

An external declaration with __attribute__((transaction_callable))
is an assertion by the user that the transactional clone exists
(or alternately, a directive from the user to generate such a clone
in the file that contains the function).

>>> @@ -2294,6 +2294,7 @@ cgraph_copy_node_for_versioning (struct
>>>     new_version->rtl = old_version->rtl;
>>>     new_version->reachable = true;
>>>     new_version->count = old_version->count;
>>> +   new_version->lowered = true;
>>
>> OTOH this isn't necessary true.  cgraph exists before lowering.

But no clones are created before lowering.


r~
Richard Guenther - Nov. 7, 2011, 9:44 a.m.
On Sun, Nov 6, 2011 at 7:53 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> [rth, more comments for you below]
>
> On 11/04/11 04:14, Richard Guenther wrote:
>
>>>    new_version = cgraph_create_node (new_decl);
>>>
>>> -   new_version->analyzed = true;
>>> +   new_version->analyzed = old_version->analyzed;
>>
>> Hm?  analyzed means "with body", sure you have a body if you clone.
>>
>>>    new_version->local = old_version->local;
>>>    new_version->local.externally_visible = false;
>>>    new_version->local.local = true;
>>> @@ -2294,6 +2294,7 @@ cgraph_copy_node_for_versioning (struct
>>>    new_version->rtl = old_version->rtl;
>>>    new_version->reachable = true;
>>>    new_version->count = old_version->count;
>>> +   new_version->lowered = true;
>>
>> OTOH this isn't necessary true.  cgraph exists before lowering.
>
> I don't understand what you want me to do on either of these two comments.
>  Could you elaborate?

Do not push new_version->lowered setting into the core routine but keep
it at the callers.

>>> +  /* TM pure functions should not get inlined if the outer function is
>>> +     a TM safe function.  */
>>> +  else if (flag_tm
>>
>> Please move flag checks into the respective prediates.  Any reason
>> why the is_tm_pure () predicate wouldn't already do the correct thing
>> with !flag_tm?
>
> Done.
>
>>> +  /* Map gimple stmt to tree label (or list of labels) for transaction
>>> +     restart and abort.  */
>>> +  htab_t GTY ((param_is (struct tm_restart_node))) tm_restart;
>>> +
>>
>> As this maps 'gimple' to tree shouldn't this go to fn->gimple_df instead?
>> That way you avoid growing generic struct function.  Or in to eh_status,
>> if that looks like a better fit.
>
> Done.
>
>>> +  /* Mark all calls that can have a transaction restart.  */
>>
>> Why isn't this done when we expand the call?  This walking of the
>> RTL sequence looks like a hack (an easy one, albeit).
>>
>>> +  if (cfun->tm_restart&&  is_gimple_call (stmt))
>>> +    {
>>> +      struct tm_restart_node dummy;
>>> +      void **slot;
>>> +
>>> +      dummy.stmt = stmt;
>>> +      slot = htab_find_slot (cfun->tm_restart,&dummy, NO_INSERT);
>>> +      if (slot)
>>> +       {
>>> +         struct tm_restart_node *n = (struct tm_restart_node *) *slot;
>>> +         tree list = n->label_or_list;
>>> +         rtx insn;
>>> +
>>> +         for (insn = next_real_insn (last); !CALL_P (insn);
>>> +              insn = next_real_insn (insn))
>>> +           continue;
>>> +
>>> +         if (TREE_CODE (list) == LABEL_DECL)
>>> +           add_reg_note (insn, REG_TM, label_rtx (list));
>>> +         else
>>> +           for (; list ; list = TREE_CHAIN (list))
>>> +             add_reg_note (insn, REG_TM, label_rtx (TREE_VALUE (list)));
>>> +       }
>>> +    }
>
> I can certainly move this to expand_call_stmt() if you prefer.  Do you have
> an objection to the RTL walk?  This isn't my code, but I'm open to
> suggestions on an alternative to implement.

It just catched my eye...  moving it to expand_call_stmt would be nice
indeed, but I was suggesting to add that note where we produce the
CALL rtx, not sure if that's reasonably straight-forward (I suppose there
was a reason to go with the hack above ...).

Richard.
Richard Henderson - Nov. 7, 2011, 4:08 p.m.
On 11/07/2011 01:44 AM, Richard Guenther wrote:
> It just catched my eye...  moving it to expand_call_stmt would be nice
> indeed, but I was suggesting to add that note where we produce the
> CALL rtx, not sure if that's reasonably straight-forward (I suppose there
> was a reason to go with the hack above ...).

Because targets emit all sorts of stuff in gen_call that isn't a CALL_INSN.
We have to search anyway.  And the guts of expand_call are complex enough
already; no need to make it worse.


r~

Patch

Index: ipa-inline.c
===================================================================
--- ipa-inline.c	(revision 181028)
+++ ipa-inline.c	(working copy)
@@ -286,8 +286,7 @@  can_inline_edge_p (struct cgraph_edge *e
     }
   /* TM pure functions should not get inlined if the outer function is
      a TM safe function.  */
-  else if (flag_tm
-	   && is_tm_pure (callee->decl)
+  else if (is_tm_pure (callee->decl)
 	   && is_tm_safe (e->caller->decl))
     {
       e->inline_failed = CIF_UNSPECIFIED;
Index: function.h
===================================================================
--- function.h	(revision 181028)
+++ function.h	(working copy)
@@ -467,14 +467,6 @@  extern GTY(()) struct rtl_data x_rtl;
    want to do differently.  */
 #define crtl (&x_rtl)
 
-/* This structure is used to map a gimple statement to a label,
-   or list of labels to represent transaction restart.  */
-
-struct GTY(()) tm_restart_node {
-  gimple stmt;
-  tree label_or_list;
-};
-
 struct GTY(()) stack_usage
 {
   /* # of bytes of static stack space allocated by the function.  */
@@ -526,10 +518,6 @@  struct GTY(()) function {
   /* Value histograms attached to particular statements.  */
   htab_t GTY((skip)) value_histograms;
 
-  /* Map gimple stmt to tree label (or list of labels) for transaction
-     restart and abort.  */
-  htab_t GTY ((param_is (struct tm_restart_node))) tm_restart;
-
   /* For function.c.  */
 
   /* Points to the FUNCTION_DECL of this function.  */
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 181028)
+++ trans-mem.c	(working copy)
@@ -172,9 +172,13 @@  get_attrs_for (const_tree x)
 bool
 is_tm_pure (const_tree x)
 {
-  tree attrs = get_attrs_for (x);
-  if (attrs)
-    return lookup_attribute ("transaction_pure", attrs) != NULL;
+  if (flag_tm)
+    {
+      tree attrs = get_attrs_for (x);
+      if (attrs)
+	return lookup_attribute ("transaction_pure", attrs) != NULL;
+      return false;
+    }
   return false;
 }
 
@@ -205,16 +209,17 @@  is_tm_irrevocable (tree x)
 bool
 is_tm_safe (const_tree x)
 {
-  tree attrs = get_attrs_for (x);
-
-  if (attrs)
+  if (flag_tm)
     {
-      if (lookup_attribute ("transaction_safe", attrs))
-	return true;
-      if (lookup_attribute ("transaction_may_cancel_outer", attrs))
-	return true;
+      tree attrs = get_attrs_for (x);
+      if (attrs)
+	{
+	  if (lookup_attribute ("transaction_safe", attrs))
+	    return true;
+	  if (lookup_attribute ("transaction_may_cancel_outer", attrs))
+	    return true;
+	}
     }
-
   return false;
 }
 
@@ -270,22 +275,6 @@  is_tm_may_cancel_outer (tree x)
   return false;
 }
 
-/* Return true if STMT may alter control flow via a transactional edge.  */
-
-bool
-is_transactional_stmt (const_gimple stmt)
-{
-  switch (gimple_code (stmt))
-    {
-    case GIMPLE_CALL:
-      return (gimple_call_flags (stmt) & ECF_TM_OPS) != 0;
-    case GIMPLE_TRANSACTION:
-      return true;
-    default:
-      return false;
-    }
-}
-
 /* Return true for built in functions that "end" a transaction.   */
 
 bool
@@ -2470,13 +2459,13 @@  make_tm_edge (gimple stmt, basic_block b
   void **slot;
   struct tm_restart_node *n, dummy;
 
-  if (cfun->tm_restart == NULL)
-    cfun->tm_restart = htab_create_ggc (31, struct_ptr_hash,
-					struct_ptr_eq, ggc_free);
+  if (cfun->gimple_df->tm_restart == NULL)
+    cfun->gimple_df->tm_restart = htab_create_ggc (31, struct_ptr_hash,
+						   struct_ptr_eq, ggc_free);
 
   dummy.stmt = stmt;
   dummy.label_or_list = gimple_block_label (region->entry_block); 
-  slot = htab_find_slot (cfun->tm_restart, &dummy, INSERT);
+  slot = htab_find_slot (cfun->gimple_df->tm_restart, &dummy, INSERT);
   n = (struct tm_restart_node *) *slot;
   if (n == NULL)
     {
Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 181028)
+++ cfgexpand.c	(working copy)
@@ -2097,13 +2097,13 @@  expand_gimple_stmt (gimple stmt)
     }
 
   /* Mark all calls that can have a transaction restart.  */
-  if (cfun->tm_restart && is_gimple_call (stmt))
+  if (cfun->gimple_df->tm_restart && is_gimple_call (stmt))
     {
       struct tm_restart_node dummy;
       void **slot;
 
       dummy.stmt = stmt;
-      slot = htab_find_slot (cfun->tm_restart, &dummy, NO_INSERT);
+      slot = htab_find_slot (cfun->gimple_df->tm_restart, &dummy, NO_INSERT);
       if (slot)
 	{
 	  struct tm_restart_node *n = (struct tm_restart_node *) *slot;
@@ -4483,7 +4483,11 @@  gimple_expand_cfg (void)
   naked_return_label = NULL;
 
   /* After expanding, the tm_restart map is no longer needed.  */
-  cfun->tm_restart = NULL;
+  if (cfun->gimple_df->tm_restart)
+    {
+      htab_delete (cfun->gimple_df->tm_restart);
+      cfun->gimple_df->tm_restart = NULL;
+    }
 
   /* Tag the blocks with a depth number so that change_scope can find
      the common parent easily.  */
Index: tree-flow.h
===================================================================
--- tree-flow.h	(revision 181028)
+++ tree-flow.h	(working copy)
@@ -33,6 +33,14 @@  along with GCC; see the file COPYING3.  
 #include "tree-ssa-alias.h"
 
 
+/* This structure is used to map a gimple statement to a label,
+   or list of labels to represent transaction restart.  */
+
+struct GTY(()) tm_restart_node {
+  gimple stmt;
+  tree label_or_list;
+};
+
 /* Gimple dataflow datastructure. All publicly available fields shall have
    gimple_ accessor defined in tree-flow-inline.h, all publicly modifiable
    fields should have gimple_set accessor.  */
@@ -80,6 +88,10 @@  struct GTY(()) gimple_df {
   unsigned int ipa_pta : 1;
 
   struct ssa_operands ssa_operands;
+
+  /* Map gimple stmt to tree label (or list of labels) for transaction
+     restart and abort.  */
+  htab_t GTY ((param_is (struct tm_restart_node))) tm_restart;
 };
 
 /* Accessors for internal use only.  Generic code should use abstraction
@@ -778,9 +790,6 @@  extern bool maybe_duplicate_eh_stmt (gim
 extern bool verify_eh_edges (gimple);
 extern bool verify_eh_dispatch_edge (gimple);
 
-/* In gtm-low.c  */
-extern bool is_transactional_stmt (const_gimple);
-
 /* In tree-ssa-pre.c  */
 struct pre_expr_d;
 void add_to_value (unsigned int, struct pre_expr_d *);
Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 181028)
+++ tree-cfg.c	(working copy)
@@ -117,6 +117,7 @@  static int gimple_verify_flow_info (void
 static void gimple_make_forwarder_block (edge);
 static void gimple_cfg2vcg (FILE *);
 static gimple first_non_label_stmt (basic_block);
+static bool verify_gimple_transaction (gimple);
 
 /* Flowgraph optimization and cleanup.  */
 static void gimple_merge_blocks (basic_block, basic_block);
@@ -4098,18 +4099,6 @@  verify_gimple_switch (gimple stmt)
   return false;
 }
 
-/* Verify the contents of a GIMPLE_TRANSACTION.  Returns true if there
-   is a problem, otherwise false.  */
-
-static bool
-verify_gimple_transaction (gimple stmt)
-{
-  tree lab = gimple_transaction_label (stmt);
-  if (lab != NULL && TREE_CODE (lab) != LABEL_DECL)
-    return true;
-  return false;
-}
-
 /* Verify a gimple debug statement STMT.
    Returns true if anything is wrong.  */
 
@@ -4339,7 +4328,7 @@  verify_gimple_in_seq_2 (gimple_seq stmts
 	  break;
 
 	case GIMPLE_TRANSACTION:
-	  err |= verify_gimple_in_seq_2 (gimple_transaction_body (stmt));
+	  err |= verify_gimple_transaction (stmt);
 	  break;
 
 	default:
@@ -4355,6 +4344,18 @@  verify_gimple_in_seq_2 (gimple_seq stmts
   return err;
 }
 
+/* Verify the contents of a GIMPLE_TRANSACTION.  Returns true if there
+   is a problem, otherwise false.  */
+
+static bool
+verify_gimple_transaction (gimple stmt)
+{
+  tree lab = gimple_transaction_label (stmt);
+  if (lab != NULL && TREE_CODE (lab) != LABEL_DECL)
+    return true;
+  return verify_gimple_in_seq_2 (gimple_transaction_body (stmt));
+}
+
 
 /* Verify the GIMPLE statements inside the statement list STMTS.  */
 
@@ -5122,9 +5123,10 @@  gimple_redirect_edge_and_branch (edge e,
     case GIMPLE_TRANSACTION:
       /* The ABORT edge has a stored label associated with it, otherwise
 	 the edges are simply redirectable.  */
-      /* ??? We don't really need this label after the cfg is created.  */
-      if (e->flags == 0)
+      /* ??? We don't really need this label after the cfg is created.
+      if (&e->flags == 0)
 	gimple_transaction_set_label (stmt, gimple_block_label (dest));
+      */
       break;
 
     default: