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

[Pan, Xiuli]

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