[llvm-commits] PATCH. Fix of hang during intel jit profiling.

[Malea, Daniel]

This patch-set looks all right to me; all the Intel-specific stuff has been sunk down as Chris requested. Now, only API-agnostic stuff remains in the include tree; everything else is under lib/ExecutionEngine/IntelJITEvents.

Any other reviewers want to take a look to see if there's anything else to be fixed or can be improved?

Dan

On 2012-08-31, at 9:16 AM, Uhanov, Kirill wrote:

> Hi Chris.
> You are right about headers in include/llvm/ExecutionEngine. Both jitprofiling.h and IntelJITEventsWrapper.h were moved to lib path.
> 
> IntelJITEvents was chosen because ITT JIT API is not technically specific to VTune.  The other Intel products may use it in the future as well.
> 
> I separated patch to 2: 
> 	- removing static dependency and import header files
> 	- fix for hang during jit profiling.
> 
> Could you review attached patches?
> 
> Thanks, Kirill
> 
> -----Original Message-----
> From: Chris Lattner [mailto:clattner at apple.com] 
> Sent: Wednesday, August 22, 2012 3:23 AM
> To: Uhanov, Kirill
> Cc: Anton Korobeynikov; llvm-commits LLVM; Pravdin, Denis; Kuanbekov, Artyom
> Subject: Re: [llvm-commits] PATCH. Fix of hang during intel jit profiling.
> 
> 
> On Aug 14, 2012, at 12:56 AM, "Uhanov, Kirill" <kirill.uhanov at intel.com> wrote:
> 
>> Hi All.
>> We got approval to contribute our files under 'the University of Illinois Open Source License' .
>> Could you review attached patch?
> 
> Great!  
> 
> +++ ./include/llvm/ExecutionEngine/jitprofiling.h	2012-08-13 18:25:25.000000000 +0400
> @@ -0,0 +1,254 @@
> +/*===-- jitprofiling.h - JIT Profiling API-------------------------*- C -*-===*
> 
> Maybe I'm not understanding the design here, but why isn't this in ExecutionEngine/IntelJITEvents?
> 
> If this needs to stay, then it is a problem that it isn't following the LLVM conventions.  If this is an imported header file, it should be sunk down into the IntelJITEvents directory and can remain relatively-unmodified.
> 
> As a random question, why is the directory named ExecutionEngine/IntelJITEvents instead of "ExecutionEngine/VTuneEvents" or something like that?  Intel has a number of JITs -- this code is an interface to VTune, not to the JITs that intel ships.
> 
> Another high level comment is that this patch includes two things: 1) importing header files, and 2) some implementation changes in IntelJITEventListener.cpp.  It would be good to separate those out to make it easier to review and understand the patch.
> 
> -Chris
> <1_import_headers.patch><2_hang_fix.patch>