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

[Jim Grosbach]

Hi Kiril,

I'm coming at this a bit late, so please bear with me if you've already answered these questions and I've just missed it.

The configure changes look fine to me, but I'm not an expert on that, so CC'ed Eric in case he wants to have a look.

As Chris mentions below, the implementations of jitprofiling.h, jitprofiling.c, ittnotify_types.h, and ittnotify_config.h don't follow the LLVM conventions. Given that you moved these down into the IntelJITEvents directory, I gather that they are imported headers from elsewhere rather than anything LLVM specific? If so, can you add a comment in the file headers to that effect? Otherwise folks (like me) will be tempted to come along and "clean" things up, making future merges more difficult for you. If, on the other hand, they are all or partly an LLVM specific implementation of a more general API, then the implementations and LLVM-side data structures should be updated to reflect the LLVM coding conventions.

The details of the bug fix look reasonable to me, so it's just a matter of the above style questions.

Regards,
  Jim


On Sep 14, 2012, at 12:09 AM, "Uhanov, Kirill" <kirill.uhanov at intel.com> wrote:

> Ping.
> 
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Uhanov, Kirill
> Sent: Wednesday, September 05, 2012 12:02 PM
> To: Chris Lattner (clattner at apple.com)
> Cc: Pravdin, Denis; llvm-commits LLVM; Kuanbekov, Artyom; Alexandrov, Alexei
> Subject: [llvm-commits] FW: PATCH. Fix of hang during intel jit profiling.
> 
> Ping.
> 
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Uhanov, Kirill
> Sent: Friday, August 31, 2012 5:17 PM
> To: Chris Lattner
> Cc: Kuanbekov, Artyom; Pravdin, Denis; Alexandrov, Alexei; llvm-commits LLVM
> Subject: Re: [llvm-commits] PATCH. Fix of hang during intel jit profiling.
> 
> 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
> 
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park, 
> 17 Krylatskaya Str., Bldg 4, Moscow 121614, 
> Russian Federation
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park, 
> 17 Krylatskaya Str., Bldg 4, Moscow 121614, 
> Russian Federation
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 
> --------------------------------------------------------------------
> Closed Joint Stock Company Intel A/O
> Registered legal address: Krylatsky Hills Business Park, 
> 17 Krylatskaya Str., Bldg 4, Moscow 121614, 
> Russian Federation
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> <1_import_headers.patch><2_hang_fix.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits