412331 - Suggested VTune annotations for checkin

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Marsha Eng]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727; InfoPath.1; MS-RTC LM 8)
Build Identifier: 305:958e6ff6b3ec

The attached include the modifications required to enable support for VTune.  All modifications are #ifdef'ed with VTUNE so a diff is straightforward.

The purpose of this checkin is to 1) show developers what's required to enable VTune, and 2) enable developers to help us figure out how to remove performance overhead due to the Debugger builds.

To actually run under VTune, you will also need the VTune header/cpp files, and static and dynamic libraries.  We're working on getting those released.

Reproducible: Always

Steps to Reproduce:
1. Copy Codegen.* to codegen directory.
2. Copy all other files to core directory.
3. hg diff
Actual Results:  
You'll see the VTUNE code.

Comment 1

[Marsha Eng]

Attachment: AvmCore.cpp with VTune annotations

AvmCore.cpp with VTune annotations

Comment 2

[Marsha Eng]

Attachment: AvmCore.h with VTune annotations

AvmCore.h with VTune annotations

Comment 3

[Marsha Eng]

Attachment: CodegenMIR.cpp with VTune annotations

CodegenMIR.cpp with VTune annotations

Comment 4

[Marsha Eng]

Attachment: CodegenMIR.h with VTune annotations

CodegenMIR.h with VTune annotations

Comment 5

[Marsha Eng]

Attachment: MethodInfo.cpp with VTune annotations

MethodInfo.cpp with VTune annotations

Comment 6

[Marsha Eng]

Attachment: Verifier.cpp with VTune annotations

Verifier.cpp with VTune annotations

Comment 7

[Benjamin Smedberg]

Marsha, it would be much more helpful if you attached your changes as a patch (generated with "hg diff") rather than individual files.

Comment 8

[Marsha Eng]

*** Here is the result of hg diff; it seems I no longer have the ability to add a patch (will remember next time). ***

diff -r 958e6ff6b3ec codegen/CodegenMIR.cpp
--- a/codegen/CodegenMIR.cpp	Fri Jan 11 21:02:06 2008 +0100
+++ b/codegen/CodegenMIR.cpp	Mon Jan 14 12:57:35 2008 -0800
@@ -38,6 +38,10 @@
 
 #include "avmplus.h"
 
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+#include "Vtune.h"
+#endif // VTUNE
+
 #ifdef DARWIN
 #include <Carbon/Carbon.h>
 #endif
@@ -137,6 +141,10 @@ return *((sintptr*)&_method);
 
 namespace avmplus
 {
+		#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+			extern "C" int iJIT_NotifyEvent(iJIT_JVM_EVENT, void *); 
+		#endif // VTUNE
+
 		sintptr CodegenMIR::profAddr( void (DynamicProfiler::*f)() )
 		{
 			RETURN_VOID_METHOD_PTR(DynamicProfiler, f);
@@ -418,7 +426,12 @@ namespace avmplus
 		if (++arg_index > call->argc)
 		{
 			lastFunctionCall = call;
+#ifdef VTUNE
+			this->ip = call + 1 + (call->argc+opsize-1)/opsize;
+#else
 			this->ip = call + (call->argc+7)/4;  // NB. DCE depends on this layout!
+#endif // VTUNE
+
 			#ifdef AVMPLUS_VERBOSE
 			if (verbose())
 				core->console << "@" << (arg ? InsNbr(arg) : -1) << ")\n";
@@ -660,7 +673,11 @@ namespace avmplus
 					 * but only check the last slot (which contains lastUse) if the instruction 
 					 * looks like a store or a call
 					 */
-					AvmAssert(sizeof(OP) == 16);
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+						AvmAssert(sizeof(OP) == 36); 
+#else
+						AvmAssert(sizeof(OP) == 16); 
+#endif // VTUNE
 					#ifdef AVMPLUS_64BIT
 					AvmAssert(0); // 64bit - needs fixes - is uintptr right for slot/currIns??
 					#endif
@@ -1337,6 +1354,12 @@ namespace avmplus
 		framep = SP;
 		interruptable = true;
 		overflow = false;
+			
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		this->cur_srcfile = NULL;	
+		this->cur_srcline = -1;
+		this->cur_methodname = NULL;
+#endif // VTUNE
 
 		#if defined(AVMPLUS_IA32) && defined(_MAC)
 		patch_esp_padding = NULL;
@@ -1356,6 +1379,12 @@ namespace avmplus
 		state = NULL;
 		framep = SP;
  		interruptable = true;
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		this->cur_srcfile = NULL;	
+		this->cur_srcline = -1;
+		this->cur_methodname = NULL;
+#endif // VTUNE
 
 		#ifdef AVMPLUS_MAC_CARBON
 		setjmpInit();
@@ -1416,6 +1445,12 @@ namespace avmplus
 		state = NULL;
 		framep = SP;
 		interruptable = true;
+		
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		this->cur_srcfile = NULL;	
+		this->cur_srcline = -1;
+		this->cur_methodname = NULL;
+#endif // VTUNE
 
 		#ifdef AVMPLUS_MAC_CARBON
 		setjmpInit();
@@ -3003,8 +3038,14 @@ namespace avmplus
 					if (off < state->pc)
 						extendLastUse(jmpt, off);
 				}
-				AvmAssert(sizeof(OP)==16); // jump table size calc relies on this
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+				AvmAssert(sizeof(OP) == 36); // jump table size calc relies on this
+				ip += (count+opsize-1)/opsize; // skip jump table
+				#else
+				AvmAssert(sizeof(OP) == 16); // jump table size calc relies on this
 				ip += (count+3)/4; // skip jump table
+#endif // VTUNE
+
 				case_count += count;
 
 				// don't want cse's to live past indirect jumps
@@ -4544,6 +4585,19 @@ namespace avmplus
 				callIns(MIR_cm, DEBUGGERADDR(Debugger::debugFile), 2,
 						debugger,
 						InsConst(op1));
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+				cur_srcfile = (wchar *)(((String *)op1) ->c_str());
+                Stringp methodName = (Stringp) info->name;
+                if (methodName) {
+					cur_methodname = (wchar*) methodName->c_str();
+					if (core->VTuneStatus == iJIT_CALLGRAPH_ON) {
+						JITedFlag = 1;
+					}
+                };
+				cur_srcline = -1;
+#endif // VTUNE
+
 				break;
 		    }
 
@@ -4555,6 +4609,11 @@ namespace avmplus
 				callIns(MIR_cm, DEBUGGERADDR(Debugger::debugLine), 2,
 						debugger,
 						InsConst(op1));
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+				cur_srcline = (int)op1;
+#endif // VTUNE
+
 				break;
 			}
 			#endif // DEBUGGER
@@ -5640,6 +5699,23 @@ namespace avmplus
 		#endif // AVMPLUS_ARM
 		
 		#ifdef AVMPLUS_IA32
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+				if ((core->VTuneStatus == iJIT_CALLGRAPH_ON) && (JITedFlag)) {
+					MDInstruction* VTmip = mip;
+					VTentryPT = (int*)(mip+1);
+
+					MOV(EAX, 0x12345678);
+					PUSH(EAX);
+					PUSH(0x13);
+			
+					void * funcptr= (void*)&iJIT_NotifyEvent ;
+					CALL((int)funcptr-((int)VTmip+13));
+
+					ADD(ESP, 8);
+				}
+#endif  //VTUNE
+
 		if (core->minstack)
 		{
 			// Check the stack
@@ -5964,6 +6040,26 @@ namespace avmplus
 					rematerialize(v);
 				}
 			}
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+				if ((core->VTuneStatus == iJIT_CALLGRAPH_ON) && (JITedFlag)) {
+					PUSH(EAX);
+
+				    MDInstruction* VTmip = mip;
+					VTexitPT = (int*)(mip+1);	
+					
+					MOV(EAX, 0x12345678);
+					PUSH(EAX);
+					PUSH(0x14);
+			
+					void * funcptr= (void*)&iJIT_NotifyEvent ;
+					CALL((int)funcptr-((int)VTmip+13));
+	
+					ADD(ESP, 8);
+					POP(EAX);
+				}
+#endif  //VTUNE
+
 
 			//ADD(ESP, arSize); 
 			//POP  (EBP);
@@ -7701,6 +7797,11 @@ namespace avmplus
 		// get rid of pages for any part of the buffer that was not used.
 		pool->codeBuffer->decommitUnused();
 
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		mymipStart = mipStart;
+		mymipEnd = mip;
+#endif // VTUNE
+
 #ifdef DEBUGGER
 		info->codeSize = int((mip - mipStart) * sizeof(MDInstruction));
 #endif
@@ -7942,6 +8043,11 @@ namespace avmplus
 		{
 			SAMPLE_CHECK();
 			MirOpcode mircode = ip->code;
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+			ip->md_startaddr = mip;
+			ip->myargc = ip->argc;
+#endif // VTUNE
 
 			#ifdef AVMPLUS_VERBOSE
 			if (verbose() && mircode != MIR_bb)
@@ -9760,7 +9866,11 @@ namespace avmplus
 					fpregs.expireAll(ip);
 
 					// skip MIR case table
+#ifdef VTUNE
+					ip += (ip->size+opsize-1)/opsize;
+#else
 					ip += (ip->size+3)/4;
+#endif // VTUNE
 
 					break;
 				}
@@ -9983,7 +10093,11 @@ namespace avmplus
 					{
 						// if it's a pure operator call and it's not used, skip it
 						int argc = ip->argc;
+#ifdef VTUNE
+						OP* postCall = ip + (argc+opsize-1)/opsize;
+#else
 						OP* postCall = ip + (argc+3)/4;
+#endif // VTUNE
 						for (int i=1; i <= argc; i++)
 						{
 							OP* argval = ip->args[i];
@@ -10006,7 +10120,11 @@ namespace avmplus
 
 					// point to call instruction
 					OP* call = ip;
+#ifdef VTUNE
+					OP* postCall = call+(argc+opsize-1)/opsize;
+#else
 					OP* postCall = call+(argc+3)/4;
+#endif // VTUNE
 
 					// buffer protection
 					const static int maxBytesPerArg = 16;
@@ -10618,4 +10736,48 @@ namespace avmplus
 	}
 #endif
 
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+	OP* CodegenMIR::getNextIp(OP* ip, int flag)
+	{
+		OP* nextip;
+
+		switch (ip->code)
+		{
+			case MIR_jmpt:
+				{
+					nextip = ip + (ip->size+opsize-1)/opsize;
+					nextip++;
+					break;
+				}
+			case MIR_cmop:
+			case MIR_csop:
+			case MIR_fcmop:
+			case MIR_fcsop:
+
+			case MIR_ci:
+			case MIR_fci:
+			case MIR_cm:
+			case MIR_cs:
+			case MIR_fcm:
+			case MIR_fcs:
+				{
+					int argc;
+					if (flag) argc = ip->myargc;
+					else argc = ip->argc;
+//					core->console << "Argc: " << argc << "\n";
+					nextip = ip + (argc+opsize-1)/opsize;
+					nextip++;
+					break;
+				}
+			default:
+					nextip = ip;
+					nextip++;
+					break;
+		}
+//		core->console << opsize << " Cur: " << ip->method_srcline << " Around: "
+//			<< (ip+1)->method_srcline << "\n";
+		return(nextip);
+
+	}
+#endif // VTUNE
 }
diff -r 958e6ff6b3ec codegen/CodegenMIR.h
--- a/codegen/CodegenMIR.h	Fri Jan 11 21:02:06 2008 +0100
+++ b/codegen/CodegenMIR.h	Mon Jan 14 12:57:35 2008 -0800
@@ -513,7 +513,13 @@ namespace avmplus
 		 * During machine dependent code generation oprnd2 is clobbered and
 		 * the current register / stack position is maintained within this field.
 		 */
-
+#ifdef VTUNE
+		// non-vtune builds can declare this later on
+		// note that we don't consider the non IA32 case
+		#ifdef AVMPLUS_IA32
+		typedef byte MDInstruction;
+		#endif // AVMPLUS_IA32
+#endif // VTUNE
 		class OP
 		{
 		public:
@@ -529,6 +535,14 @@ namespace avmplus
 
 			/** link to a previous expr with the same opcode, for finding CSE's */
 			uint32 prevcse:16;
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+			const wchar* method_srcfile;
+			int	  method_srcline;
+			MDInstruction* md_startaddr;
+			wchar* method_name;
+			uint32 myargc;
+#endif  // VTUNE
 
 			union
 			{
@@ -573,6 +587,10 @@ namespace avmplus
 
 		};
 
+#ifdef VTUNE
+		static const int opsize = sizeof(OP) / sizeof(OP*);
+#endif // VTUNE
+
 		class MirLabel
 		{
 		public:
@@ -605,7 +623,16 @@ namespace avmplus
 
 		void emitMD();
 
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		OP* getNextIp(OP* ip, int flag);
+#endif // VTUNE
+
 		OP* exAtom;
+#ifdef VTUNE
+		// vtune needs these to be public
+		OP* ipStart;
+		OP* ipEnd;
+#endif // VTUNE
 
 	private:
 		#define PROFADDR(f) profAddr((void (DynamicProfiler::*)())(&f))
@@ -644,8 +671,12 @@ namespace avmplus
 
 		// MIR instruction buffer
 		OP* ip;
+#ifndef VTUNE
+		// if no vtune, these can be private
 		OP* ipStart;
 		OP* ipEnd;
+#endif // !VTUNE
+
 		uintptr mirBuffSize;
 		int expansionFactor;
 		GrowableBuffer* mirBuffer;
@@ -653,6 +684,20 @@ namespace avmplus
 		byte*	 code;
 		uintptr*  casePtr;
 		int		 case_count;
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		wchar* cur_srcfile;
+		int   cur_srcline;
+		wchar* cur_methodname;
+
+		public:MDInstruction* mymipStart;
+		public:MDInstruction* mymipEnd;
+
+		// CG support
+		public:int  JITedFlag;
+		public:int* VTentryPT;
+		public:int* VTexitPT;
+#endif // VTUNE
 
 		#ifdef AVMPLUS_PPC
 		typedef uint32 MDInstruction;
@@ -664,7 +709,10 @@ namespace avmplus
 		#endif
 		
 		#ifdef AVMPLUS_IA32
+#ifndef VTUNE
+		// vtune build requires this defn in time for OP class defn
 		typedef byte MDInstruction;
+#endif // !VTUNE
 		#define PREV_MD_INS(m) (m-4)
 		// for intel and our purposes previous instruction is 4 bytes prior to m; used for patching 32bit target addresses
 		#endif
@@ -1646,8 +1694,14 @@ namespace avmplus
     #endif /* AVMPLUS_ARM */
 	
 	#ifdef AVMPLUS_IA32
+#ifdef VTUNE
+	static const int md_prologue_size		= 64;
+	static const int md_epilogue_size		= 256;
+#else
 	static const int md_prologue_size		= 32;
 	static const int md_epilogue_size		= 128;
+#endif // VTUNE
+
 	static const int md_native_thunk_size	= 256;
 	#endif /* AVMPLUS_PPC */
 	
diff -r 958e6ff6b3ec core/AvmCore.cpp
--- a/core/AvmCore.cpp	Fri Jan 11 21:02:06 2008 +0100
+++ b/core/AvmCore.cpp	Mon Jan 14 12:57:35 2008 -0800
@@ -155,6 +155,10 @@ namespace avmplus
 			#endif
 
 		#endif // AVMPLUS_MIR
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+			VTuneStatus = CheckVTuneStatus();
+#endif // VTUNE
 
 		interrupts = false;
 
diff -r 958e6ff6b3ec core/AvmCore.h
--- a/core/AvmCore.h	Fri Jan 11 21:02:06 2008 +0100
+++ b/core/AvmCore.h	Mon Jan 14 12:57:35 2008 -0800
@@ -37,6 +37,10 @@
 
 #ifndef __avmplus_AvmCore__
 #define __avmplus_AvmCore__
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+#include "Vtune.h"
+#endif // VTUNE
 
 namespace avmplus
 {
@@ -97,6 +101,16 @@ const int kBufferPadding = 16;
 		 * setConsoleStream.
 		 */
 		PrintWriter console;
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		iJIT_IsProfilingActiveFlags VTuneStatus;
+
+		iJIT_IsProfilingActiveFlags CheckVTuneStatus() {
+//			iJIT_RegisterCallbackEx(NULL, NULL);
+			iJIT_IsProfilingActiveFlags profiler = iJIT_IsProfilingActive();	
+			return profiler;
+		}
+#endif // VTUNE
 
 		/**
 		 * The GC used by this AVM instance
diff -r 958e6ff6b3ec core/MethodInfo.cpp
--- a/core/MethodInfo.cpp	Fri Jan 11 21:02:06 2008 +0100
+++ b/core/MethodInfo.cpp	Mon Jan 14 12:57:35 2008 -0800
@@ -38,9 +38,18 @@
 
 #include "avmplus.h"
 
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+#include "Vtune.h"
+#endif // VTUNE
+
 namespace avmplus
 {
 	using namespace MMgc;
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+	extern void Vtune_Profile(CodegenMIR *, AvmCore *); 
+#endif  // VTUNE
+
 	MethodInfo::MethodInfo()
 		: AbstractFunction()
 	{
@@ -94,9 +103,46 @@ namespace avmplus
 		if (core->turbo && !isFlagSet(AbstractFunction::SUGGEST_INTERP))
 		{
 			CodegenMIR mir(this);
+			
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+				
+#ifdef VTUNE_DEBUGGER
+					core->console << "VTuneStatus = "<< core->VTuneStatus <<"\n";
+#endif // VTUNE_DEBUGGER
+
+				if (core->VTuneStatus == iJIT_CALLGRAPH_ON)  {
+					mir.JITedFlag = 0;   
+				}
+#endif // VTUNE
+		
 			verifier.verify(&mir);	// pass 2 - data flow
+
+#ifndef VTUNE
 			if (!mir.overflow)
 				mir.emitMD(); // pass 3 - generate code
+#else
+			if (!mir.overflow) {
+				mir.emitMD(); // pass 3 - generate code
+#ifdef VTUNE_DEBUGGER
+				core->console << "\n##################################################################\n";
+				CodegenMIR::OP * cur_ip = mir.ipStart;
+				while (cur_ip < mir.ipEnd)
+				{
+					if ((cur_ip->method_srcfile!=NULL) && (cur_ip->method_srcline!=-1)){
+						core->console << "MIR @	"<< (int)cur_ip << "		MIROpcode:"<<(int)cur_ip->code<<"\n";
+						core->console << "		###SrcFile:     "<< cur_ip->method_srcfile <<"\n";
+						core->console << "		###SrcLine:     "<< cur_ip->method_srcline <<"\n";
+						core->console << "		###MethodName:  "<< cur_ip->method_name <<"\n";
+						core->console << "		###MDstartAddr: "<< (int)cur_ip->md_startaddr <<"\n";
+						core->console << "	^^^^mir.mipStart: " << (int)mir.mymipStart << "	mir.mipEnd: "<< (int)mir.mymipEnd << "\n";
+					}
+					cur_ip = mir.getNextIp(cur_ip, 1);
+				}
+				core->console << "\n\n";
+#endif // VTUNE_DEBUGGER
+			}
+			Vtune_Profile(&mir, core);
+#endif // !VTUNE
 
 			// the MD buffer can overflow so we need to re-iterate
 			// over the whole thing, since we aren't yet robust enough
diff -r 958e6ff6b3ec core/Verifier.cpp
--- a/core/Verifier.cpp	Fri Jan 11 21:02:06 2008 +0100
+++ b/core/Verifier.cpp	Mon Jan 14 12:57:35 2008 -0800
@@ -264,6 +264,22 @@ namespace avmplus
 		}
 
 		int size;
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+		CodegenMIR::OP*	savedip = NULL;
+		if (mir) {
+			savedip = mir->ipStart;
+			while (savedip < mir->ip) {
+				savedip->method_srcfile = NULL;
+				savedip->method_srcline = -1;
+				savedip->method_name = NULL;
+
+				savedip = mir->getNextIp(savedip, 0);
+			}
+			AvmAssert(savedip == mir->ip);
+		}
+#endif // VTUNE
+
 		for (const byte* pc = code_pos, *code_end=code_pos+code_length; pc < code_end; pc += size)
 		{
 			SAMPLE_CHECK();
@@ -271,6 +287,18 @@ namespace avmplus
 			if (core->dprof.dprofile)
 				core->dprof.mark(OP_verifyop);
 			#endif
+
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+			if (mir) {
+				while (savedip < mir->ip) { 
+					savedip->method_srcfile = mir->cur_srcfile;
+					savedip->method_srcline = mir->cur_srcline;
+					savedip->method_name = mir->cur_methodname;
+					savedip->md_startaddr = 0;
+					savedip = mir->getNextIp(savedip, 0);
+				}
+			}
+#endif // VTUNE
 
 			if (mir && mir->overflow)
 			{
@@ -2664,6 +2692,15 @@ namespace avmplus
 		#endif //AVMPLUS_INTERP
 		{
 			mir->epilogue(state);
+#if defined (VTUNE) || defined (VTUNE_DEBUGGER)
+				while (savedip < mir->ipEnd) {
+					savedip->method_srcfile = NULL;
+					savedip->method_srcline = -1;
+					savedip->method_name = NULL;
+					savedip = mir->getNextIp(savedip, 0);
+				}
+				AvmAssert(savedip == mir->ipEnd);
+#endif // VTUNE
 		}
 
 		#ifdef FEATURE_BUFFER_GUARD

Comment 9

[Rick Reitmaier]

Marsha would you mind to try again since I cut and paste doesn't work with the above diffs (doesn't produce a valid patch file).  Or you can send me the bundle/patch directly if you like.

Comment 10

[Marsha Eng]

Attachment: VTune additions patch

Bear with me...I'm still learning bugzilla

Comment 11

[Tommy Reilly]

As a general rule we don't write anything to core->console unless there's a flag telling us to (ie verbose flag).  

Comment 12

[Marsha Eng]

Attachment: Revised patch

Here are the suggested vtune modifications with printouts and other extraneous stuff removed.

Comment 13

[Rick Reitmaier]

Comment on attachment 297275 [details] [diff] [review]
Revised patch

CodegenMIR.h:540 Is there another technique we can use, adding 20Bytes to OP implies that each instruction takes a hit.  It might not be too bad since this is only the IR but still it does effectively double the size of an OP.  

Could we build up a SortedIntMap containing pointers to the debug_line/debug_file opcodes or something like that?  Its not clear from the patch how the data collected is used.

CodegenMIR.h:630  Could we create public accessor fncs to expose ipStart/End.  They could then be ifdef'd based on VTUNE

CodegenMIR.h:710 Although more invasive let's move the whole MDInstruction clump (PPC/ARM/IA32, etc) to the start of the file so we can avoid more ifdefs.

MethodInfo.cpp:115 We probably only want to profile when mir is successful.  So maybe something like :
			if (!mir.overflow)
			{
				mir.emitMD(); // pass 3 - generate code
#ifdef VTUNE
				Vtune_Profile(&mir, core);
#endif // VTUNE
			}

Codegen.cpp:430  Your change looks safer than what we are using in general.  Can we replace the original code?  Likewise at 3045 and a few other locations.

Comment 14

[Marsha Eng]

I am working on a patch to address all the other comments except

CodegenMIR.h:540
  Vtune_Profile iterates over all ip's from mir->ipStart to mir->ipEnd (that's why we want them to be public).  A state machine decides when to register methods with VTune.  It uses getNextIP to iterate over the list.
  Shengnan's code for the state machine is fairly complex.  It appears that she encounters instructions that fall outside the stated address range or that have invalid information (like an address but no method name).  Every instruction has this additional payload so we can check for validity/sanity every time so as to ensure VTune has precise information.
  I really like the idea of an opcode/instruction/method map to do method lookups at registration time.  I am refactoring Shengnan's code at the moment and hope to have a clear idea of the exceptions she's seeing soon.
  FWIW, I have measured no discernible performance difference between the VTune and Release_Debugger builds on the few js-speed benchmarks under my radar.  Therefore the optimization is one of size, and we'll tackle that next.

Comment 15

[Marsha Eng]

Attachment: Revision #2 to VTune patch

Addresses all issues except increase in OP size.  If this patch is OK, please check in.

The next patch will have the filenames corrected for the actual library distribution and modifications to the build files to enable a VTune build.

Comment 16

[Rick Reitmaier]

Re: public ipStart/End.  Not sure if I was clear but I was saying that we should have public getters for ipStart/End rather than expose the variable.   And if the values are being modified outside the class, let's add setters too.

On the OP size increase; I think we want to converge on a final design/implementation prior to landing the patch.  I'm eager to get the changes in, but I'd hate to do it prematurely.  What do you think?

Finally, regarding the release and release debugger issue; is it possible to apply the changes to the release builds?  Release Debugger builds generate extra JIT'd code that would be good to avoid for performance tuning.

Comment 17

[Edwin Smith]

How about adding MIR_line and MIR_file, which are inserted inbetween other MIR OP's as abc is translated to MIR.  The semantics are that every instruction following MIR_line or MIR_file is treated as if it had the line/file info itself.

Then during the assembly pass, the position tables can be populated with the current line and file, with the current line/file being updated by the MIR ops.

if necessary for translation, an additional MIR_file/line OP could be inserted at the beginning or end of each basic block.

Comment 18

[Marsha Eng]

Attachment: Patch including suggested changes to VS8 project files

Comment 19

[Marsha Eng]

Attachment: New file vtune.cpp

This is new code required to do the actual method registration with VTune.  vtune.cpp resides in the core directory.

Comment 20

[Marsha Eng]

Unless I've misunderstood the meaning of "public getters" for ipStart/End, my understanding is VTune is the only build that reads these variables outside the class.  VTune doesn't have to set these outside the class (it just reads whatever value), which is why I've avoided adding public setters.  If you think public get/set functions would be useful in general, they don't have to reside within the #ifdefs.  (Note that getNextIp, the iteration function, may also be generally useful at some point, but for now is only required by VTune.)

I've tried adding the VTune changes to the Release build instead of Release_Debugger with little success.  This is why I've asked for a Debugger tutorial at Friday's summit.  Once I understand the Debugger a little better I can tell you exactly where the dependence is coming from.  If Ed's suggestion of MIR_file and MIR_line is viable, this may make it easier to remove the dependence on Debugger assuming it's possible to populate these fields without the Debugger.

It sounds like MIR_line and MIR_file essentially shift the payload out of OP, but it's still there.  After refactoring vtune.cpp, I understand Shengnan's original code a little better.  She has two conditions for capturing a method: 1) if the next instruction has a source file different from the previous one, or 2) we have reached the end of the instruction stream.

In addition to MIR_line, we'd also need something like MIR_address.  VTune captures both source line number of a particular instruction as well as its address offset into the method.  I suppose we could, based on a starting address/line number, assume each instruction is at a fixed offset and compute accordingly.  However, Shengnan has checks in her code to adjust the starting point, and I know this code is being called.  I take it this means each OP will need to be responsible for adjusting MIR_*, effectively shifting the work out of vtune_profile.

I think the net effect will be a nominal decrease in payload size, but as I've stated before, this already seems to have no discernible impact on performance.

Comment 21

[Rick Reitmaier]

Attachment: changes for vtune support in release builds.

Marsha, here are the changes for building the table in both release and release debugger builds that should be somewhat compatible with Vtune.cpp

Can you please attach the VTune.h file ; I only see the .cpp.  After that, we'll be able to figure out what we'll need to tweak in Vtune.cpp to get it all up and running.

Comment 22

[Marsha Eng]

Thanks, I'm merging and checking the code right now.  It looks like I need to add back the code to enable callgraph as well as vtune.cpp.

VTune.h is now JITProfiling.h and should be included with the library bundle from your Premier account.  Included in the previous patch are modifications to the project files to look in the right place for this header file, assuming standard installation.  Let me know if you're missing this file.

Comment 23

[Rick Reitmaier]

Sorry maybe I wasn't clear, but my patch is not complete.  Its a work in progress.   I just sent you an email regarding some questions/comments, once we converge I'll rev the patch again.

re: JITProfiling.h file thanks for the info

Comment 24

[Rick Reitmaier]

Attachment: changes for vtune support in release builds take 2

Compiles and runs, but not yet tested against VTune.  Probably its pretty close right now, all we'll need is some testing :)

I've included it as a bundle since I didn't yet merge to latest.  You'll need to do an 'hg pull vtune.bundle' to get the changes.  

Let me know how it goes.

Comment 25

[Rick Reitmaier]

Attachment: latest patch: bundle containing all changes + 2 bug fixes (malloc and wtoc call)

bundle containing all changes + 2 bug fixes (malloc and wtoc call)

hg in vtuneRevII.bundle     <= to see the list of changes
hg pull -u vtuneRevII.bundle   <= to get the changes 

make sure you DONT merge, you'll see 2 heads (of you're sync'd to lasest) and you need to update to the one in the bundle.  Let me know if this doesn't make sense and I can walk you through it.

BTW, thanks Moh for the help !

Also, making all prior rev's obsolete.

Comment 26

[Shengnan Cong]

Attachment: VTune patches

Address issues with VTune Callgraph:
1. Make changes to VTune.cpp to disable the write-protection on iJIT_Method_NIDS;
2. Fix a bug in CodegenMIR.cpp for VTune exiting (Line 6037);

VTune should work fine for both Sampling and Callgraph now.

-Shengnan

Comment 27

[Rick Reitmaier]

Attachment: consolidated changes

latest patch: contains Shengnan's mods + project file changes for vc2008.  Use  'hg in vtuneRevIII.bundle' to pull it in.

Comment 28

[Rick Reitmaier]

Attachment: final(?) bundle of changes

Incorporated project file changes, backported to VS2005 and final wrap up (we hope )

You'll need to pull the latest from tamarin and then 'hg in vtuneRevIV.bundle' to apply this single change.  Please test under VS2005 thanks.

Comment 29

[Rick Reitmaier]

Comment on attachment 307087 [details]
consolidated changes

obsolete

Comment 30

[Marsha Eng]

Built with VS2005. Passes sampling and callgraph. As expected, the only modification needed is the path to the header and static library.

I'm still having problems with the review flags, hence the comment.

Comment 31

[Shengnan Cong]

Tested under VS2005. Require to copy the VTune JIT supporting package (including header and libs)into module/vtune in the parent directory of tamarin-central. Both Sampling and Callgraph worked fine. 

I am also having problem with the review flags. I set the '?' to a '+' but got "You are not authorized to edit attachment 307139 [details]" after submitting the comments.

Comment 32

[Rick Reitmaier]

Comment on attachment 307139 [details]
final(?) bundle of changes

marsha review and approved.

Comment 33

[Rick Reitmaier]

Comment on attachment 307139 [details]
final(?) bundle of changes

shengnan marked '+' but then she went away.

Comment 34

[Edwin Smith]

some comments...  and by the way these could be followin work items, not
necessarily a pre-requisite for the initial push.

in DEBUGGER config, Does it make any difference whether MIR_line/file are
emitted just before, or just after, the calls to debugLine() and debugFile()?

would it make any sense to generate the calls to iJIT_NotifyEvent() as a
MIR_call, rather than explicit assembly code to do it?  if there's any benefit
at all, it might be just in the maintenance, or in having the assembler take
care of spilling around the call.  since the current approach works, maybe not
a worthwhile change.

should we have instructions for updating the VS2005 config to point to the
vtune header/lib dirs, so users don't have to modify the code itself to find
these files?

could the vtune ifdefs in AvmCore.cpp be moved over to VTune.cpp?

Comment 35

[Rick Reitmaier]

comments re Ed's points:

re: DEBUGGER config; currently we don't have a project that enables DEBUGGER and VTUNE at the same time.  If this we think this will be needed, then MIR_line/file positioning may make become important.

re: iJIT_NotifyEvent() .  The issue here is that we want the calls to occur as close to the actual top of method entry/exit as possible so that all the method overheads are accounted for and not just MIR generated code.

re: good idea we need to update the build instructions.  Moh, can you take on this work item?

re: we could move the ifdefs, but I'm not sure how we'd call into VTune.cpp in a  non-ifdef'd fashion.  If you have some specific change/style you're looking to make let me know and we can fold it in.

Comment 36

[Moh Haghighat]

I'll add to the wiki a short writeup on the instructions for VTune-build configuration and VTune project creation (sampling & call-graph). I'll, however, be out of office tomorrow, Thursday. 

Comment 37

[Moh Haghighat]

Comment on attachment 307139 [details]
final(?) bundle of changes

a long overdue +