436987 - VTune support for TT

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tracing Virtual Machine, defect)

Description

[Shengnan Cong]

I have added the VTune sampling support for TT jitted code. With this functionality, you are able to see the mixed AS source and the JITted assembly code. This would be helpful in generating efficient code.

Comment 1

[Shengnan Cong]

Attachment: Patch 1

Comment 2

[Shengnan Cong]

Attachment: Addtional file: VTune.cpp

Comment 3

[Shengnan Cong]

I added LIR_file & LIR_line to the LIR dictionary. For SETFILE, SETLINE, PUSHFRAME and POPFRAME, LIR_file/line are inserted. 

Comment 4

[Shengnan Cong]

Attachment: Patch to the latest TT

Comment 5

[Rick Reitmaier]

The overall approach looks good and the patch is shaping up nicely.

I have a few comments / questions:

- can we make LIR_line/file available in all builds and just ifdef out the functionallity.  This also means that we'd get rid of the code added to ignoreInstruction().
- vm_fpu_xxx are generated functions and thus any changes applied to them are transient.  Have you talked to Steven about adding support for what you need?
- as this is a work in progress there are a few places where code is commented out.
- It wasn't clear to me why you needed to access/modify StringNullTerminatedUTF8; could String() simply be used.

Comment 6

[Steven Johnson]

Yeah, vm_fpu_xxx are all machine-generated and shouldn't be modified. You want to add the code in question to the relevant parts of prim.fs and re-run the Forth compiler (just run core/builtin.py). If this isn't obvious then contact me for more info.

Comment 7

[Shengnan Cong]

Attachment: changes to LIR according to Rick's comments

Comment 8

[Shengnan Cong]

Thanks for the comments.

- I made some changes to LIR.h/.cpp and also ignoreInstruction() in the lastest patch. I am not sure whether this is what you mean by making LIR_file/line available in all builds. The changes in LIR.h/.cpp are not guarded by ifdefs and should be available elsewhere.

- I have tried to add code to prim.fs. I am having the following problems:
  1. There is no EOL generated after #ifdef VTUNE. 
  2. I need to add codes to both the interp part and trace part of a prim in prim.fs, but only want these added code generated in vm_XXX_trace.h, not in vm_XXX_interp.h. Is there a way to do that?

- The code that is commented out may be necessary for VTune Callgraph. I leave the comments there just in case we need to extend it for Callgraph later.

- I am using StringNullTerminatedUTF8 since getData(), getLen() of String are protected and I didn't want to change that.
 

Comment 9

[Steven Johnson]

re: prim.fs:
1. #ifdefs currently don't work so well, the workaround we've used is to define macros like AVMPLUS_VERBOSE_ONLY() to enclose conditional code. We should add a similar AVMPLUS_VTUNE_ONLY macro to avmbuild.h
2. Currently, anything you add to the interp part will go into the trace part as well.... and this is by design as we want the same code to be executed. Can you perhaps expand on why you can't just put everything in the trace section?

re: String, it's slated for major changes soon anyway, so using the ugly UTF8 wrapper class sounds fine for now.

Comment 10

[Shengnan Cong]

The reason for adding to the interp part is because in PRIMs SETFILE and SETLINE, f -> filename/linenum are set in the interp part. I insert LIR_file/line after the setting. Anyway, I have fixed it in the new patch. Thanks, Steven.

Comment 11

[Shengnan Cong]

Attachment: latest patch: changes made to fix the problems in the comments from Rick and Steven

change to prim.fs and avmbuild.h.

Comment 12

[Steven Johnson]

What the story on the commented-out line: (Interpreter.cpp 2037 or so)

    //if (vtune && !overflow)

ditto for some similar lines in Assembler.h and .cpp -- if they are meant to be commented out permanently, we should remove them. (Unless they are meant to show what NOT to do, in which case a further comment would be appropriate)

I'd prefer not to have the "using namespace avmplus" in Assembler.h under an ifdef -- this could cause some subtle compile differences between VTune and non-VTune builds. Let's either add it unconditionally or explicitly quality the stuff that needs avmplus.

Comment 13

[Shengnan Cong]

Attachment: latest

The field vtune in Assembler is not refered now for VTune Sampling, but it would be used for for Callgraph feature if it is going to be extended, so does the commented statment. I have removed these comments for now.

I am now adding "using namespace avmplus" unconditionally in Assembler.h.

Comment 14

[Shengnan Cong]

Attachment: latest VTune.cpp

did some clean-up of the code

Comment 15

[Shengnan Cong]

I am merging the patch for the latest TT. It seems to me that SETFILE/SETLINE are not set in the ForthIL of the latest TT. I generated .abc with -d and set breakpoints at 
INTERP_FOPCODE_INTERP_BEGIN(SETFILE) and INTERP_FOPCODE_INTERP_BEGIN(SETLINE) in vm_fpu_trace.h. But the program never stops. It seems that SETFILE/SETLINE are not emitted. Am I missing anything? Thanks.

Comment 16

[Edwin Smith]

In the latest TT, we do not emit calls OP_debugline and OP_debugfile in non-debugger builds.  SETLINE and SETFILE are called from those words.  However,
if you look for the ifdef around OP_debugline in LIR.cpp, you can have it emit SETLINE.

but... you may not need to.  TT now has code to create an address/file/line mapping table for IL code that we generate.  Perhaps the vtune tables could be set up in a very similar fashion.  Take a look at updateLine() in IL.cpp.

When we generate IL, we build a mapping of IL address ranges to "labels" which are a string containing function, file, line.  

Could a similar table (mapping IL address ranges to file/line) be consulted during tracing to keep track of file/line for LIR instructions, and finally machine instructions?  

(if this is too complicated, we can also just ensure SETLINE is emitted like before)

Comment 17

[Shengnan Cong]

Edwin, thanks for the information. I believe you meant OP_debugline in IL.cpp. 

It seems that using a map similar to labelmap may require re-writing most of my previous implementation. Can I just emit SETFILE/SETLINE guarded with "#ifdef VTUNE" in verifier to avoid the complication?

Comment 18

[Edwin Smith]

certianly, labelmap was just an idea to look at.  emitting SETFILE/LINE should be fine.

Comment 19

[Shengnan Cong]

Attachment: patch for the latest TT

Comment 20

[Shengnan Cong]

Attachment: latest VTune.cpp

Comment 21

[Shengnan Cong]

I just uploaded the patches for the latest TT. Changes are made to use the new LIR and String. SETFILE/LINE are emitted guarded by #ifdef VTUNE in verifier.

Need to push the code soon so that no more merges. 

Comment 22

[Shengnan Cong]

Attachment: patch for the latest TT

Comment 23

[Shengnan Cong]

Comment on attachment 330846 [details] [diff] [review]
latest VTune.cpp

The code looks fine and I mark "+" for it.

But I just get a fresh copy of the latest TT and applied the patch to it. It seems that the VTune project is not setting the path to JITProfiling.h.

Comment 24

[Rick Reitmaier]

thanks for checking the code Shengnan.  I'm currently working on cleaning that up along with a few other bits that should be available for review shortly.  I'll post it here.

Comment 25

[Edwin Smith]

i think this landed?

Comment 26

[Rick Reitmaier]

yes it did, closing.