[PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Patrick Marlier]

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

In PR51280, LTO does ICE because the object file uses TM builtin but the 
TM is not enabled.
In the patch, it displays a error message if the builtin is not defined 
and due to TM.
I moved is_tm_builtin() from calls.c to trans-mem.c. I splitted it into 
2 functions is_tm_builtin/is_tm_builtin_code. In is_tm_builtin_code, I 
added some missing builtins (TM_START, TM_GETTMCLONE_SAFE, TM_MALLOC, 
TM_CALLOC, TM_FREE). Finally, I declared them into tree.h to be usable 
in calls.c and tree-streamer-in.c.

Bootstrapped and LTO/TM regtested on Linux/i686.
(If ok, please commit it. Thanks.)

Patrick Marlier.


[-- Attachment #2: pr51280.Changelog --]
[-- Type: text/plain, Size: 329 bytes --]

2011-12-15  Patrick Marlier  <patrick.marlier@gmail.com>

	PR lto/51280
	* tree.h (is_tm_builtin): Declare.
	(is_tm_builtin_code): Declare.
	* calls.c (is_tm_builtin): Move to...
	* trans-mem.c (is_tm_builtin): ...here.
	(is_tm_builtin_code): New, add missing builtins.
	* lto-streamer-in.c (streamer_get_builtin_tree): Use it.


[-- Attachment #3: pr51280.patch --]
[-- Type: text/plain, Size: 5569 bytes --]

Index: tree.h
===================================================================
--- tree.h	(revision 182376)
+++ tree.h	(working copy)
@@ -5871,6 +5871,8 @@ extern bool is_tm_safe (const_tree);
 extern bool is_tm_pure (const_tree);
 extern bool is_tm_may_cancel_outer (tree);
 extern bool is_tm_ending_fndecl (tree);
+extern bool is_tm_builtin_code (enum built_in_function);
+extern bool is_tm_builtin (const_tree);
 extern void record_tm_replacement (tree, tree);
 extern void tm_malloc_replacement (tree);
 
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 182376)
+++ trans-mem.c	(working copy)
@@ -406,6 +406,83 @@ is_tm_abort (tree fndecl)
 	  && DECL_FUNCTION_CODE (fndecl) == BUILT_IN_TM_ABORT);
 }
 
+/* Return TRUE if FCODE is a TM builtin function.  */
+
+bool
+is_tm_builtin_code (enum built_in_function fcode)
+{
+  switch (fcode)
+    {
+      case BUILT_IN_TM_START:
+      case BUILT_IN_TM_COMMIT:
+      case BUILT_IN_TM_COMMIT_EH:
+      case BUILT_IN_TM_ABORT:
+      case BUILT_IN_TM_IRREVOCABLE:
+      case BUILT_IN_TM_MEMCPY:
+      case BUILT_IN_TM_MEMMOVE:
+      case BUILT_IN_TM_MEMSET:
+      case BUILT_IN_TM_GETTMCLONE_IRR:
+      case BUILT_IN_TM_GETTMCLONE_SAFE:
+      case BUILT_IN_TM_MALLOC:
+      case BUILT_IN_TM_CALLOC:
+      case BUILT_IN_TM_FREE:
+      CASE_BUILT_IN_TM_STORE (1):
+      CASE_BUILT_IN_TM_STORE (2):
+      CASE_BUILT_IN_TM_STORE (4):
+      CASE_BUILT_IN_TM_STORE (8):
+      CASE_BUILT_IN_TM_STORE (FLOAT):
+      CASE_BUILT_IN_TM_STORE (DOUBLE):
+      CASE_BUILT_IN_TM_STORE (LDOUBLE):
+      CASE_BUILT_IN_TM_STORE (M64):
+      CASE_BUILT_IN_TM_STORE (M128):
+      CASE_BUILT_IN_TM_STORE (M256):
+      CASE_BUILT_IN_TM_LOAD (1):
+      CASE_BUILT_IN_TM_LOAD (2):
+      CASE_BUILT_IN_TM_LOAD (4):
+      CASE_BUILT_IN_TM_LOAD (8):
+      CASE_BUILT_IN_TM_LOAD (FLOAT):
+      CASE_BUILT_IN_TM_LOAD (DOUBLE):
+      CASE_BUILT_IN_TM_LOAD (LDOUBLE):
+      CASE_BUILT_IN_TM_LOAD (M64):
+      CASE_BUILT_IN_TM_LOAD (M128):
+      CASE_BUILT_IN_TM_LOAD (M256):
+      case BUILT_IN_TM_LOG:
+      case BUILT_IN_TM_LOG_1:
+      case BUILT_IN_TM_LOG_2:
+      case BUILT_IN_TM_LOG_4:
+      case BUILT_IN_TM_LOG_8:
+      case BUILT_IN_TM_LOG_FLOAT:
+      case BUILT_IN_TM_LOG_DOUBLE:
+      case BUILT_IN_TM_LOG_LDOUBLE:
+      case BUILT_IN_TM_LOG_M64:
+      case BUILT_IN_TM_LOG_M128:
+      case BUILT_IN_TM_LOG_M256:
+	return true;
+      default:
+	break;
+    }
+  return false;
+
+}
+
+/* Return TRUE if FNDECL is either a TM builtin or a TM cloned
+   function.  Return FALSE otherwise.  */
+
+bool
+is_tm_builtin (const_tree fndecl)
+{
+  if (fndecl == NULL)
+    return false;
+
+  if (decl_is_tm_clone (fndecl))
+    return true;
+
+  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL
+      && is_tm_builtin_code (DECL_FUNCTION_CODE (fndecl)))
+    return true;
+  return false;
+}
+
 /* Build a GENERIC tree for a user abort.  This is called by front ends
    while transforming the __tm_abort statement.  */
 
Index: calls.c
===================================================================
--- calls.c	(revision 182376)
+++ calls.c	(working copy)
@@ -611,69 +611,6 @@ alloca_call_p (const_tree exp)
   return false;
 }
 
-/* Return TRUE if FNDECL is either a TM builtin or a TM cloned
-   function.  Return FALSE otherwise.  */
-
-static bool
-is_tm_builtin (const_tree fndecl)
-{
-  if (fndecl == NULL)
-    return false;
-
-  if (decl_is_tm_clone (fndecl))
-    return true;
-
-  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
-    {
-      switch (DECL_FUNCTION_CODE (fndecl))
-	{
-	case BUILT_IN_TM_COMMIT:
-	case BUILT_IN_TM_COMMIT_EH:
-	case BUILT_IN_TM_ABORT:
-	case BUILT_IN_TM_IRREVOCABLE:
-	case BUILT_IN_TM_GETTMCLONE_IRR:
-	case BUILT_IN_TM_MEMCPY:
-	case BUILT_IN_TM_MEMMOVE:
-	case BUILT_IN_TM_MEMSET:
-	CASE_BUILT_IN_TM_STORE (1):
-	CASE_BUILT_IN_TM_STORE (2):
-	CASE_BUILT_IN_TM_STORE (4):
-	CASE_BUILT_IN_TM_STORE (8):
-	CASE_BUILT_IN_TM_STORE (FLOAT):
-	CASE_BUILT_IN_TM_STORE (DOUBLE):
-	CASE_BUILT_IN_TM_STORE (LDOUBLE):
-	CASE_BUILT_IN_TM_STORE (M64):
-	CASE_BUILT_IN_TM_STORE (M128):
-	CASE_BUILT_IN_TM_STORE (M256):
-	CASE_BUILT_IN_TM_LOAD (1):
-	CASE_BUILT_IN_TM_LOAD (2):
-	CASE_BUILT_IN_TM_LOAD (4):
-	CASE_BUILT_IN_TM_LOAD (8):
-	CASE_BUILT_IN_TM_LOAD (FLOAT):
-	CASE_BUILT_IN_TM_LOAD (DOUBLE):
-	CASE_BUILT_IN_TM_LOAD (LDOUBLE):
-	CASE_BUILT_IN_TM_LOAD (M64):
-	CASE_BUILT_IN_TM_LOAD (M128):
-	CASE_BUILT_IN_TM_LOAD (M256):
-	case BUILT_IN_TM_LOG:
-	case BUILT_IN_TM_LOG_1:
-	case BUILT_IN_TM_LOG_2:
-	case BUILT_IN_TM_LOG_4:
-	case BUILT_IN_TM_LOG_8:
-	case BUILT_IN_TM_LOG_FLOAT:
-	case BUILT_IN_TM_LOG_DOUBLE:
-	case BUILT_IN_TM_LOG_LDOUBLE:
-	case BUILT_IN_TM_LOG_M64:
-	case BUILT_IN_TM_LOG_M128:
-	case BUILT_IN_TM_LOG_M256:
-	  return true;
-	default:
-	  break;
-	}
-    }
-  return false;
-}
-
 /* Detect flags (function attributes) from the function decl or type node.  */
 
 int
Index: tree-streamer-in.c
===================================================================
--- tree-streamer-in.c	(revision 182376)
+++ tree-streamer-in.c	(working copy)
@@ -1055,6 +1055,8 @@ streamer_get_builtin_tree (struct lto_input_block
       if (fcode >= END_BUILTINS)
 	fatal_error ("machine independent builtin code out of range");
       result = builtin_decl_explicit (fcode);
+      if (!result && !flag_tm && is_tm_builtin_code (fcode))
+	fatal_error ("transactional memory builtin without support enabled");
       gcc_assert (result);
     }
   else if (fclass == BUILT_IN_MD)

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Richard Guenther]

On Thu, 15 Dec 2011, Patrick Marlier wrote:

> In PR51280, LTO does ICE because the object file uses TM builtin but the TM is
> not enabled.
> In the patch, it displays a error message if the builtin is not defined and
> due to TM.
> I moved is_tm_builtin() from calls.c to trans-mem.c. I splitted it into 2
> functions is_tm_builtin/is_tm_builtin_code. In is_tm_builtin_code, I added
> some missing builtins (TM_START, TM_GETTMCLONE_SAFE, TM_MALLOC, TM_CALLOC,
> TM_FREE). Finally, I declared them into tree.h to be usable in calls.c and
> tree-streamer-in.c.
> 
> Bootstrapped and LTO/TM regtested on Linux/i686.
> (If ok, please commit it. Thanks.)

No - why should this matter?  All of TM should be lowered to a point
where only target specific code should be needed.

Richard.

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Patrick Marlier]

On 12/16/2011 03:54 AM, Richard Guenther wrote:
> On Thu, 15 Dec 2011, Patrick Marlier wrote:
>
>> In PR51280, LTO does ICE because the object file uses TM builtin but the TM is
>> not enabled.
>> In the patch, it displays a error message if the builtin is not defined and
>> due to TM.
>> I moved is_tm_builtin() from calls.c to trans-mem.c. I splitted it into 2
>> functions is_tm_builtin/is_tm_builtin_code. In is_tm_builtin_code, I added
>> some missing builtins (TM_START, TM_GETTMCLONE_SAFE, TM_MALLOC, TM_CALLOC,
>> TM_FREE). Finally, I declared them into tree.h to be usable in calls.c and
>> tree-streamer-in.c.
>>
>> Bootstrapped and LTO/TM regtested on Linux/i686.
>> (If ok, please commit it. Thanks.)
>
> No - why should this matter?  All of TM should be lowered to a point
> where only target specific code should be needed.
>
> Richard.

Thanks Richard.

In lto file, there is GIMPLE_TRANSACTION statement and a builtin call 
(__builtin_ITM_commitTransaction) to delimit the end of the transaction 
region. The transaction is not yet instrumented. So all of TM are not 
lowered.

I guess this could be also added even if we should always break at the 
missing _ITM_commitTransaction builtin declaration.

Index: gimple-streamer-in.c
===================================================================
--- gimple-streamer-in.c        (revision 182487)
+++ gimple-streamer-in.c        (working copy)
@@ -234,6 +234,9 @@ input_gimple_stmt (struct lto_input_block *ib, str
        break;

      case GIMPLE_TRANSACTION:
+      if (!flag_tm)
+        error_at (gimple_location (stmt),
+                 "use of transactional memory without support enabled");
        gimple_transaction_set_label (stmt, stream_read_tree (ib, data_in));
        break;


It seems a bit out of my scope of my GCC knowledge so I guess I will let 
GCC guys solve this in a proper way.

Patrick.

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Richard Guenther]

On Mon, 19 Dec 2011, Patrick Marlier wrote:

> On 12/16/2011 03:54 AM, Richard Guenther wrote:
> > On Thu, 15 Dec 2011, Patrick Marlier wrote:
> > 
> > > In PR51280, LTO does ICE because the object file uses TM builtin but the
> > > TM is
> > > not enabled.
> > > In the patch, it displays a error message if the builtin is not defined
> > > and
> > > due to TM.
> > > I moved is_tm_builtin() from calls.c to trans-mem.c. I splitted it into 2
> > > functions is_tm_builtin/is_tm_builtin_code. In is_tm_builtin_code, I added
> > > some missing builtins (TM_START, TM_GETTMCLONE_SAFE, TM_MALLOC, TM_CALLOC,
> > > TM_FREE). Finally, I declared them into tree.h to be usable in calls.c and
> > > tree-streamer-in.c.
> > > 
> > > Bootstrapped and LTO/TM regtested on Linux/i686.
> > > (If ok, please commit it. Thanks.)
> > 
> > No - why should this matter?  All of TM should be lowered to a point
> > where only target specific code should be needed.
> > 
> > Richard.
> 
> Thanks Richard.
> 
> In lto file, there is GIMPLE_TRANSACTION statement and a builtin call
> (__builtin_ITM_commitTransaction) to delimit the end of the transaction
> region. The transaction is not yet instrumented. So all of TM are not lowered.
> 
> I guess this could be also added even if we should always break at the missing
> _ITM_commitTransaction builtin declaration.
> 
> Index: gimple-streamer-in.c
> ===================================================================
> --- gimple-streamer-in.c        (revision 182487)
> +++ gimple-streamer-in.c        (working copy)
> @@ -234,6 +234,9 @@ input_gimple_stmt (struct lto_input_block *ib, str
>        break;
> 
>      case GIMPLE_TRANSACTION:
> +      if (!flag_tm)
> +        error_at (gimple_location (stmt),
> +                 "use of transactional memory without support enabled");
>        gimple_transaction_set_label (stmt, stream_read_tree (ib, data_in));
>        break;
> 
> 
> It seems a bit out of my scope of my GCC knowledge so I guess I will let GCC
> guys solve this in a proper way.

I'd say we should stream the new IL elements and enable TM at link-time
once we encounter such IL element (similar to how we enable exceptions
when one TU contains EH regions).

Richard.

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Aldy Hernandez]

On 12/20/11 03:43, Richard Guenther wrote:
> On Mon, 19 Dec 2011, Patrick Marlier wrote:
>
>> On 12/16/2011 03:54 AM, Richard Guenther wrote:
>>> On Thu, 15 Dec 2011, Patrick Marlier wrote:
>>>
>>>> In PR51280, LTO does ICE because the object file uses TM builtin but the
>>>> TM is
>>>> not enabled.
>>>> In the patch, it displays a error message if the builtin is not defined
>>>> and
>>>> due to TM.
>>>> I moved is_tm_builtin() from calls.c to trans-mem.c. I splitted it into 2
>>>> functions is_tm_builtin/is_tm_builtin_code. In is_tm_builtin_code, I added
>>>> some missing builtins (TM_START, TM_GETTMCLONE_SAFE, TM_MALLOC, TM_CALLOC,
>>>> TM_FREE). Finally, I declared them into tree.h to be usable in calls.c and
>>>> tree-streamer-in.c.
>>>>
>>>> Bootstrapped and LTO/TM regtested on Linux/i686.
>>>> (If ok, please commit it. Thanks.)
>>>
>>> No - why should this matter?  All of TM should be lowered to a point
>>> where only target specific code should be needed.
>>>
>>> Richard.
>>
>> Thanks Richard.
>>
>> In lto file, there is GIMPLE_TRANSACTION statement and a builtin call
>> (__builtin_ITM_commitTransaction) to delimit the end of the transaction
>> region. The transaction is not yet instrumented. So all of TM are not lowered.
>>
>> I guess this could be also added even if we should always break at the missing
>> _ITM_commitTransaction builtin declaration.
>>
>> Index: gimple-streamer-in.c
>> ===================================================================
>> --- gimple-streamer-in.c        (revision 182487)
>> +++ gimple-streamer-in.c        (working copy)
>> @@ -234,6 +234,9 @@ input_gimple_stmt (struct lto_input_block *ib, str
>>         break;
>>
>>       case GIMPLE_TRANSACTION:
>> +      if (!flag_tm)
>> +        error_at (gimple_location (stmt),
>> +                 "use of transactional memory without support enabled");
>>         gimple_transaction_set_label (stmt, stream_read_tree (ib, data_in));
>>         break;
>>
>>
>> It seems a bit out of my scope of my GCC knowledge so I guess I will let GCC
>> guys solve this in a proper way.
>
> I'd say we should stream the new IL elements and enable TM at link-time
> once we encounter such IL element (similar to how we enable exceptions
> when one TU contains EH regions).

Ok, I was finally able to reproduce with the new testcase Patrick 
provided.  I'm back on the saddle on this one.

Richi, I can certainly do something similar here, but let me see if 
we're on the same wavelength before I commit many keystrokes.

What I have in mind is to abstract out the initialization of TM builtins 
from gtm-builtins.def (through its inclusion in builtins.def) into a 
separate function that we can call either in c_define_builtins() from 
the C-ish front-ends, or from lto_define_builtins (once we encounter a 
TM builtin and enable flag_tm appropriately).

We may also have to add a hook to initialize target dependent TM 
builtins (for example, ix86_init_tm_builtins on x86).

Is this acceptable or did you have something else in mind?

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Richard Guenther]

On Tue, 17 Jan 2012, Aldy Hernandez wrote:

> On 12/20/11 03:43, Richard Guenther wrote:
> > On Mon, 19 Dec 2011, Patrick Marlier wrote:
> > 
> > > On 12/16/2011 03:54 AM, Richard Guenther wrote:
> > > > On Thu, 15 Dec 2011, Patrick Marlier wrote:
> > > > 
> > > > > In PR51280, LTO does ICE because the object file uses TM builtin but
> > > > > the
> > > > > TM is
> > > > > not enabled.
> > > > > In the patch, it displays a error message if the builtin is not
> > > > > defined
> > > > > and
> > > > > due to TM.
> > > > > I moved is_tm_builtin() from calls.c to trans-mem.c. I splitted it
> > > > > into 2
> > > > > functions is_tm_builtin/is_tm_builtin_code. In is_tm_builtin_code, I
> > > > > added
> > > > > some missing builtins (TM_START, TM_GETTMCLONE_SAFE, TM_MALLOC,
> > > > > TM_CALLOC,
> > > > > TM_FREE). Finally, I declared them into tree.h to be usable in calls.c
> > > > > and
> > > > > tree-streamer-in.c.
> > > > > 
> > > > > Bootstrapped and LTO/TM regtested on Linux/i686.
> > > > > (If ok, please commit it. Thanks.)
> > > > 
> > > > No - why should this matter?  All of TM should be lowered to a point
> > > > where only target specific code should be needed.
> > > > 
> > > > Richard.
> > > 
> > > Thanks Richard.
> > > 
> > > In lto file, there is GIMPLE_TRANSACTION statement and a builtin call
> > > (__builtin_ITM_commitTransaction) to delimit the end of the transaction
> > > region. The transaction is not yet instrumented. So all of TM are not
> > > lowered.
> > > 
> > > I guess this could be also added even if we should always break at the
> > > missing
> > > _ITM_commitTransaction builtin declaration.
> > > 
> > > Index: gimple-streamer-in.c
> > > ===================================================================
> > > --- gimple-streamer-in.c        (revision 182487)
> > > +++ gimple-streamer-in.c        (working copy)
> > > @@ -234,6 +234,9 @@ input_gimple_stmt (struct lto_input_block *ib, str
> > >         break;
> > > 
> > >       case GIMPLE_TRANSACTION:
> > > +      if (!flag_tm)
> > > +        error_at (gimple_location (stmt),
> > > +                 "use of transactional memory without support enabled");
> > >         gimple_transaction_set_label (stmt, stream_read_tree (ib,
> > > data_in));
> > >         break;
> > > 
> > > 
> > > It seems a bit out of my scope of my GCC knowledge so I guess I will let
> > > GCC
> > > guys solve this in a proper way.
> > 
> > I'd say we should stream the new IL elements and enable TM at link-time
> > once we encounter such IL element (similar to how we enable exceptions
> > when one TU contains EH regions).
> 
> Ok, I was finally able to reproduce with the new testcase Patrick provided.
> I'm back on the saddle on this one.
> 
> Richi, I can certainly do something similar here, but let me see if we're on
> the same wavelength before I commit many keystrokes.
> 
> What I have in mind is to abstract out the initialization of TM builtins from
> gtm-builtins.def (through its inclusion in builtins.def) into a separate
> function that we can call either in c_define_builtins() from the C-ish
> front-ends, or from lto_define_builtins (once we encounter a TM builtin and
> enable flag_tm appropriately).

Hm?  They are included in builtins.def, that looks appropriate for
middle-end builtins.  Thus they should be initialized by lto1, too.
Are they not?

> We may also have to add a hook to initialize target dependent TM builtins (for
> example, ix86_init_tm_builtins on x86).

There is one, targetm.builtin_decl - the targets should simply initialize
them on-demand (which is for what that target-hook is designed for).

> Is this acceptable or did you have something else in mind?

Not sure I fully understood the issue with the existing gtm-builtins.def
setup.

Richard.

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Aldy Hernandez]

>> What I have in mind is to abstract out the initialization of TM builtins from
>> gtm-builtins.def (through its inclusion in builtins.def) into a separate
>> function that we can call either in c_define_builtins() from the C-ish
>> front-ends, or from lto_define_builtins (once we encounter a TM builtin and
>> enable flag_tm appropriately).
>
> Hm?  They are included in builtins.def, that looks appropriate for
> middle-end builtins.  Thus they should be initialized by lto1, too.
> Are they not?

Since flag_tm is NULL, when lto_define_builtins runs, the TM builtins do 
not get initialized.  This is because DEF_TM_BUILTIN is predicated on 
flag_tm.

It isn't until streamer_get_builtin_tree() that we realize the object 
file has a TM builtin.

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Richard Guenther]

On Tue, 17 Jan 2012, Aldy Hernandez wrote:

> 
> > > What I have in mind is to abstract out the initialization of TM builtins
> > > from
> > > gtm-builtins.def (through its inclusion in builtins.def) into a separate
> > > function that we can call either in c_define_builtins() from the C-ish
> > > front-ends, or from lto_define_builtins (once we encounter a TM builtin
> > > and
> > > enable flag_tm appropriately).
> > 
> > Hm?  They are included in builtins.def, that looks appropriate for
> > middle-end builtins.  Thus they should be initialized by lto1, too.
> > Are they not?
> 
> Since flag_tm is NULL, when lto_define_builtins runs, the TM builtins do not
> get initialized.  This is because DEF_TM_BUILTIN is predicated on flag_tm.

Ok, so arrange that -fgnu-tm is enabled at link-time as soon as it was
enabled in one TU.  You can do that alongside handling of OPT_fPIC, etc.
in lto-wrapper.c:merge_and_complain.

> It isn't until streamer_get_builtin_tree() that we realize the object file has
> a TM builtin.

I see.  Probably similar issues exist with -fopenmp and the GOMP
builtins.

Richard.

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Aldy Hernandez]

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

On 01/18/12 03:09, Richard Guenther wrote:
> On Tue, 17 Jan 2012, Aldy Hernandez wrote:
>
>>
>>>> What I have in mind is to abstract out the initialization of TM builtins
>>>> from
>>>> gtm-builtins.def (through its inclusion in builtins.def) into a separate
>>>> function that we can call either in c_define_builtins() from the C-ish
>>>> front-ends, or from lto_define_builtins (once we encounter a TM builtin
>>>> and
>>>> enable flag_tm appropriately).
>>>
>>> Hm?  They are included in builtins.def, that looks appropriate for
>>> middle-end builtins.  Thus they should be initialized by lto1, too.
>>> Are they not?
>>
>> Since flag_tm is NULL, when lto_define_builtins runs, the TM builtins do not
>> get initialized.  This is because DEF_TM_BUILTIN is predicated on flag_tm.
>
> Ok, so arrange that -fgnu-tm is enabled at link-time as soon as it was
> enabled in one TU.  You can do that alongside handling of OPT_fPIC, etc.
> in lto-wrapper.c:merge_and_complain.

The following patch fixes the problem.

OK?

>> It isn't until streamer_get_builtin_tree() that we realize the object file has
>> a TM builtin.
>
> I see.  Probably similar issues exist with -fopenmp and the GOMP
> builtins.

It is my understanding that openmp is lowered before LTO streaming, so 
this isn't a problem for it.  I manually tested something similar to the 
testcase in the patch below (but for openmp) and verified that openmp 
does not suffer from this problem.

Thanks for the suggestion.

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 1802 bytes --]

	PR lto/51280
	* lto-wrapper.c (run_gcc): Pass -fgnu_tm on.
	(merge_and_complain): Same.

Index: testsuite/gcc.dg/lto/trans-mem-3_0.c
===================================================================
--- testsuite/gcc.dg/lto/trans-mem-3_0.c	(revision 0)
+++ testsuite/gcc.dg/lto/trans-mem-3_0.c	(revision 0)
@@ -0,0 +1,8 @@
+/* { dg-lto-options {{-flto}} } */
+/* { dg-lto-do link } */
+
+/* Test that we can build one object file with -fgnu-tm
+   (trans-mem-3_1.c), but do the final link of all objects without
+   -fgnu-tm.  */
+
+int i;
Index: testsuite/gcc.dg/lto/trans-mem-3_1.c
===================================================================
--- testsuite/gcc.dg/lto/trans-mem-3_1.c	(revision 0)
+++ testsuite/gcc.dg/lto/trans-mem-3_1.c	(revision 0)
@@ -0,0 +1,18 @@
+/* { dg-options "-fgnu-tm" } */
+
+extern int i;
+
+main()
+{
+  __transaction_atomic { i = 0; }
+}
+
+#define dummy(func)							\
+  __attribute__((noinline,noclone,used)) void func() { asm (""); }
+
+dummy(_ITM_beginTransaction)
+dummy(_ITM_commitTransaction)
+dummy(_ITM_WU4)
+dummy(_ITM_WU8)
+dummy(_ITM_registerTMCloneTable)
+dummy(_ITM_deregisterTMCloneTable)
Index: lto-wrapper.c
===================================================================
--- lto-wrapper.c	(revision 183244)
+++ lto-wrapper.c	(working copy)
@@ -403,6 +403,7 @@ merge_and_complain (struct cl_decoded_op
 	case OPT_fpie:
 	case OPT_fcommon:
 	case OPT_fexceptions:
+	case OPT_fgnu_tm:
 	  /* Do what the old LTO code did - collect exactly one option
 	     setting per OPT code, we pick the first we encounter.
 	     ???  This doesn't make too much sense, but when it doesn't
@@ -555,6 +556,7 @@ run_gcc (unsigned argc, char *argv[])
 	case OPT_fpie:
 	case OPT_fcommon:
 	case OPT_fexceptions:
+	case OPT_fgnu_tm:
 	  break;
 
 	default:

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Richard Guenther]

On Wed, 18 Jan 2012, Aldy Hernandez wrote:

> On 01/18/12 03:09, Richard Guenther wrote:
> > On Tue, 17 Jan 2012, Aldy Hernandez wrote:
> > 
> > > 
> > > > > What I have in mind is to abstract out the initialization of TM
> > > > > builtins
> > > > > from
> > > > > gtm-builtins.def (through its inclusion in builtins.def) into a
> > > > > separate
> > > > > function that we can call either in c_define_builtins() from the C-ish
> > > > > front-ends, or from lto_define_builtins (once we encounter a TM
> > > > > builtin
> > > > > and
> > > > > enable flag_tm appropriately).
> > > > 
> > > > Hm?  They are included in builtins.def, that looks appropriate for
> > > > middle-end builtins.  Thus they should be initialized by lto1, too.
> > > > Are they not?
> > > 
> > > Since flag_tm is NULL, when lto_define_builtins runs, the TM builtins do
> > > not
> > > get initialized.  This is because DEF_TM_BUILTIN is predicated on flag_tm.
> > 
> > Ok, so arrange that -fgnu-tm is enabled at link-time as soon as it was
> > enabled in one TU.  You can do that alongside handling of OPT_fPIC, etc.
> > in lto-wrapper.c:merge_and_complain.
> 
> The following patch fixes the problem.
> 
> OK?

Ok.

Thanks,
Richard.

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Aldy Hernandez]

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


> PR lto/51916
> * lto-object.c (LTO_SEGMENT_NAME): Define segment name.
> (lto_obj_file_open): Use it.

Patrick, the changelog was referring to the wrong file and the wrong 
function.  I've fixed it.  I also inlined the segment name, as Ian had 
in the PR.

Richard Guenther approved the patch on the PR, so I am committing and 
closing the PR.  The final version of the patch is attached.

Thanks for fixing this!

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 604 bytes --]

	PR lto/51916
	* lto-wrapper.c (run_gcc): Pass the LTO section name to
	simple_object_start_read.

Index: lto-wrapper.c
===================================================================
--- lto-wrapper.c	(revision 183432)
+++ lto-wrapper.c	(working copy)
@@ -479,7 +479,8 @@ run_gcc (unsigned argc, char *argv[])
       fd = open (argv[i], O_RDONLY);
       if (fd == -1)
 	continue;
-      sobj = simple_object_start_read (fd, file_offset, NULL, &errmsg, &err);
+      sobj = simple_object_start_read (fd, file_offset, "__GNU_LTO", 
+	  			       &errmsg, &err);
       if (!sobj)
 	{
 	  close (fd);

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Iain Sandoe]

Hi Patrick,

thanks for doing this,

On 21 Jan 2012, at 22:57, Patrick Marlier wrote:

> Dominique or Iain, may I ask you to test this patch on darwin? I  
> have a sporadic access to a darwin machine.

bootstrapped on i686-darwin9 - tests in progress...

minor observations:
> 	PR lto/51916
> 	* lto-object.c (LTO_SEGMENT_NAME): Define segment name.
	* lto-wrapper.c (

> 	(lto_obj_file_open): Use it.
	(run_gcc):

cheers
Iain

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Dominique Dhumieres]

> Dominique or Iain, may I ask you to test this patch on darwin? I have a
> sporadic access to a darwin machine.

The patch fixes the PR without regression on x86_64-apple-darwin10.
Bootstraping on powerpc-apple-darwin9 will finish in a couple hours.

Thanks for the debugging and the patch,

Dominique

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Patrick Marlier]

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

On 01/20/2012 08:04 AM, Dominique Dhumieres wrote:
>> The following patch fixes the problem.
>
> The test fails on *-apple-darwin*: pr51916.
>
> TIA
>
> Dominique

Dominique or Iain, may I ask you to test this patch on darwin? I have a 
sporadic access to a darwin machine.

Bootstrapped and tested on x86_64-unknown-linux-gnu.
--
Patrick.

	PR lto/51916
	* lto-object.c (LTO_SEGMENT_NAME): Define segment name.
	(lto_obj_file_open): Use it.

[-- Attachment #2: darwin-lto-opts.patch --]
[-- Type: text/x-patch, Size: 866 bytes --]

Index: lto-wrapper.c
===================================================================
--- lto-wrapper.c	(revision 183345)
+++ lto-wrapper.c	(working copy)
@@ -54,6 +54,11 @@ along with GCC; see the file COPYING3.  If not see
 
 /* End of lto-streamer.h copy.  */
 
+/* Segment name for LTO sections.  This is only used for Mach-O.
+   FIXME: This needs to be kept in sync with darwin.c.  */
+
+#define LTO_SEGMENT_NAME "__GNU_LTO"
+
 int debug;				/* true if -save-temps.  */
 int verbose;				/* true if -v.  */
 
@@ -479,7 +484,8 @@ run_gcc (unsigned argc, char *argv[])
       fd = open (argv[i], O_RDONLY);
       if (fd == -1)
 	continue;
-      sobj = simple_object_start_read (fd, file_offset, NULL, &errmsg, &err);
+      sobj = simple_object_start_read (fd, file_offset, LTO_SEGMENT_NAME,
+				       &errmsg, &err);
       if (!sobj)
 	{
 	  close (fd);

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Patrick Marlier]

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

On 01/20/2012 08:04 AM, Dominique Dhumieres wrote:
>> The following patch fixes the problem.
>
> The test fails on *-apple-darwin*: pr51916.
>
> TIA
>
> Dominique

Dominique or Iain, may I ask you to test this patch on darwin? I have a 
sporadic access to a darwin machine.

Bootstrapped and tested on x86_64-unknown-linux-gnu.
--
Patrick.

	PR lto/51916
	* lto-object.c (LTO_SEGMENT_NAME): Define segment name.
	(lto_obj_file_open): Use it.

[-- Attachment #2: darwin-lto-opts.patch --]
[-- Type: text/x-patch, Size: 866 bytes --]

Index: lto-wrapper.c
===================================================================
--- lto-wrapper.c	(revision 183345)
+++ lto-wrapper.c	(working copy)
@@ -54,6 +54,11 @@ along with GCC; see the file COPYING3.  If not see
 
 /* End of lto-streamer.h copy.  */
 
+/* Segment name for LTO sections.  This is only used for Mach-O.
+   FIXME: This needs to be kept in sync with darwin.c.  */
+
+#define LTO_SEGMENT_NAME "__GNU_LTO"
+
 int debug;				/* true if -save-temps.  */
 int verbose;				/* true if -v.  */
 
@@ -479,7 +484,8 @@ run_gcc (unsigned argc, char *argv[])
       fd = open (argv[i], O_RDONLY);
       if (fd == -1)
 	continue;
-      sobj = simple_object_start_read (fd, file_offset, NULL, &errmsg, &err);
+      sobj = simple_object_start_read (fd, file_offset, LTO_SEGMENT_NAME,
+				       &errmsg, &err);
       if (!sobj)
 	{
 	  close (fd);

Re: [PATCH] PR51280 LTO/trans-mem ICE with TM builtins

[Dominique Dhumieres]

> The following patch fixes the problem.

The test fails on *-apple-darwin*: pr51916.

TIA

Dominique