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

[Uhanov, Kirill]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1_import_headers.patch
Type: application/octet-stream
Size: 67393 bytes
Desc: 1_import_headers.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120905/c99fa81f/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2_hang_fix.patch
Type: application/octet-stream
Size: 3294 bytes
Desc: 2_hang_fix.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120905/c99fa81f/attachment-0001.obj>