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

[Richard Guenther <rguenther at suse dot de>]

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.