[Beignet] [PATCH] GBE: fix printf class static variable bug

[Yang, Rong R]

Use the thread_local make android build hard to port. Actually, it just pass the printf pass's information to GenWrite, you could use unit for it.

> -----Original Message-----
> From: Pan, Xiuli
> Sent: Tuesday, November 10, 2015 13:30
> To: Pan, Xiuli; Song, Ruiling; beignet at lists.freedesktop.org
> Cc: Yang, Rong R
> Subject: RE: [Beignet] [PATCH] GBE: fix printf class static variable bug
> 
> Ping for pushed.
> The getAnalysis seems doesn't work for this situation. Maybe we should
> refine these passes to pass info by unit later.
> 
> Thanks
> Pan Xiuli
> 
> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> Pan, Xiuli
> Sent: Friday, November 6, 2015 11:27 AM
> To: Song, Ruiling <ruiling.song at intel.com>; beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH] GBE: fix printf class static variable bug
> 
> Yes, but the problem is that if two thread has kernel with printf functions,
> the map<CallInst *, PrintfSet::PrintfFmt*> printfs will be cleared in
> construction and destructor. This will cause the one who is still need info in
> printfs get null pointer.
> thread_local now is to protect printfs from other thread but not pass data
> from one pass to another.
> 
> I tried the getAnalysis, but what we need the printfs as you said is in
> GenWriter and It could not be used there as easier as now.
> Every passmanger will clear the printfs and need a new map to avoid disturb
> each other. It is just ok when only one pass exist, but in multithread there
> will be a lot of passes in the same time. If using interface like:
>  *PrintfParser::getPrintfInfo() { return &printfs; } It still a static one, and
> threads may have disturb.
> 
> 
> -----Original Message-----
> From: Song, Ruiling
> Sent: Friday, November 6, 2015 9:54 AM
> To: Pan, Xiuli <xiuli.pan at intel.com>; Pan, Xiuli <xiuli.pan at intel.com>;
> beignet at lists.freedesktop.org
> Subject: RE: [Beignet] [PATCH] GBE: fix printf class static variable bug
> 
> Thread_local is not needed to pass data from one llvm pass to another.
> You can still access the info after pass that has already run.
> In a later llvm pass, you can use getAnalysis<PrintfParser>() to get the the
> PrintfParser pass handle.
> 
> Then expose an interface in PrintfParser then you can query the printfInfo in
> GenWriter.
> 
> Thanks!
> Ruiling
> > -----Original Message-----
> > From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf
> > Of Pan, Xiuli
> > Sent: Friday, November 6, 2015 9:44 AM
> > To: Pan, Xiuli; beignet at lists.freedesktop.org
> > Subject: Re: [Beignet] [PATCH] GBE: fix printf class static variable
> > bug
> >
> > Ping for review.
> >
> > -----Original Message-----
> > From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf
> > Of Pan Xiuli
> > Sent: Tuesday, November 3, 2015 11:30 AM
> > To: beignet at lists.freedesktop.org
> > Cc: Pan, Xiuli <xiuli.pan at intel.com>
> > Subject: [Beignet] [PATCH] GBE: fix printf class static variable bug
> >
> > The PrintfParse::printfs static is not thread safe and maybe reset or
> > adding something wrong when runing in mutlithread. Fix the problem by
> > change the printfs to a thread local variable.
> >
> > Signed-off-by: Pan Xiuli <xiuli.pan at intel.com>
> > ---
> >  backend/src/llvm/llvm_printf_parser.cpp | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/backend/src/llvm/llvm_printf_parser.cpp
> > b/backend/src/llvm/llvm_printf_parser.cpp
> > index bdaed8a..93f87ea 100644
> > --- a/backend/src/llvm/llvm_printf_parser.cpp
> > +++ b/backend/src/llvm/llvm_printf_parser.cpp
> > @@ -45,6 +45,8 @@ namespace gbe
> >  {
> >    using namespace ir;
> >
> > +  thread_local map<CallInst*, PrintfSet::PrintfFmt*> printfs;
> > +
> >    /* Return the conversion_specifier if succeed, -1 if failed. */
> >    static char __parse_printf_state(char *begin, char *end, char**
> > rend, PrintfState * state)
> >    {
> > @@ -301,7 +303,6 @@ error:
> >      Value* g1Xg2Xg3;
> >      Value* wg_offset;
> >      int out_buf_sizeof_offset;
> > -    static map<CallInst*, PrintfSet::PrintfFmt*> printfs;
> >      int printf_num;
> >      int totalSizeofSize;
> >
> > @@ -972,12 +973,10 @@ error:
> >      return false;
> >    }
> >
> > -  map<CallInst*, PrintfSet::PrintfFmt*> PrintfParser::printfs;
> > -
> >    void* getPrintfInfo(CallInst* inst)
> >    {
> > -    if (PrintfParser::printfs[inst])
> > -      return (void*)PrintfParser::printfs[inst];
> > +    if (printfs[inst])
> > +      return (void*)printfs[inst];
> >      return NULL;
> >    }
> >
> > --
> > 2.1.4
> >
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet