| Message ID | 4EEF8CB9.4060806@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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?
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.
>> 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.
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.
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;