607244 - nanojit: add LIR_comment

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: NJ patch (against TM 56283:3e513dbf0882)

I spend heaps of time looking at LIR dumps.  A big fraction of that time I spend trying to work out the mapping from TM bytecodes to LIR.  CSE makes this particularly tricky.  Sometimes I even modify the output, inserting comments.

I'm sick of this, and it can be automated.  This patch adds a new opcode, LIR_comment, which allows comments to be inserted into the LIR stream.

Here's an example from the "readlir" dumping phase:

  65: ------------------------------ # begin-getelem(dense-array)
  66: ldi10 = ldi.objclasp $stack6[4]
  67: clasp = immi 0x83afae0
  68: guard(class is Array) = eqi ldi10, clasp/*0x83afae0*/
  69: xf7: xf guard(class is Array) -> pc=0xa0fbe56 imacpc=(nil) sp+64 rp+0 (Gua
rdID=005)
  70: capacity = ldi.objcapacity $stack6[32]
  71: ltui3 = ltui subxovi1, capacity
  72: xf6: xf ltui3 -> pc=0xa0fbe56 imacpc=(nil) sp+64 rp+0 (GuardID=006)
  73: immi6 = immi 3
  74: lshi3 = lshi subxovi1, immi6/*3*/
  75: slots = ldi.objslots $stack6[36]
  76: addi1 = addi slots, lshi3
  77: ldi9 = ldi.slots addi1[4]
  78: JSVAL_TAG_INT32 = immi 0xffff0001
  79: eqi11 = eqi ldi9, JSVAL_TAG_INT32/*0xffff0001*/
  80: xf5: xf eqi11 -> pc=0xa0fbe56 imacpc=(nil) sp+64 rp+0 (GuardID=007)
  81: ldi8 = ldi.slots addi1[0]
  82: i2d4 = i2d ldi8
  83: ------------------------------ # end-getelem(dense-array)


Here's an example from the "afterdce" phase:

  ------------------------------ # begin-getelem(dense-array)
      ldi10 = ldi.objclasp $stack6[4]
      clasp = immi 0x83afae0
      guard(class is Array) = eqi ldi10, clasp/*0x83afae0*/
      xf7: xf guard(class is Array) -> pc=0xa0fbe56 imacpc=(nil) sp+64 rp+0 (Gua
rdID=005)
      capacity = ldi.objcapacity $stack6[32]
      ltui3 = ltui subxovi1, capacity
      xf6: xf ltui3 -> pc=0xa0fbe56 imacpc=(nil) sp+64 rp+0 (GuardID=006)
      immi6 = immi 3
      lshi3 = lshi subxovi1, immi6/*3*/
      slots = ldi.objslots $stack6[36]
      addi1 = addi slots, lshi3
      ldi9 = ldi.slots addi1[4]
      JSVAL_TAG_INT32 = immi 0xffff0001
      eqi11 = eqi ldi9, JSVAL_TAG_INT32/*0xffff0001*/
      xf5: xf eqi11 -> pc=0xa0fbe56 imacpc=(nil) sp+64 rp+0 (GuardID=007)
      ldi8 = ldi.slots addi1[0]
      i2d4 = i2d ldi8
  ------------------------------ # end-getelem(dense-array)

I found the big '---' line and putting the comment on the right made it easier to read.  I also deliberately de-indented the '---' line on the "afterdce" phase, I found that was nice too.

There's an argument to be made for including LIR_comment only in debug builds, I'm open to hearing it.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: TM patch (against TM 56283:3e513dbf0882)

This patch modifies jstracer.cpp to use LIR_comment in a few salient places.

Comment 2

[Edwin Smith]

Comment on attachment 485995 [details] [diff] [review]
NJ patch (against TM 56283:3e513dbf0882)

Nice feature.

Making the opcode #ifdef NJ_VERBOSE would be okay, I don't think its necessary to make it #ifdef DEBUG.  For example, Tamarin has one configuration that enables VERBOSE but not DEBUG.

Comment 3

[David Mandelin [:dmandelin]]

Comment on attachment 485997 [details] [diff] [review]
TM patch (against TM 56283:3e513dbf0882)

Neat idea!

Comment 4

[Rick Reitmaier]

Nice!   One suggestion, since LIR_comment can be any string can we integrate your comment-line into the string arg ?  

Something like :

 #define COMMENT_LINE "--------------------------------\n"

 insComment(COMMENT_LINE "begin-interruptFlags-check");


and the processing of LIR_comment becomes simply:

  VMPI_snprintf(s, n, "%s", (char*)i->oprnd1());

This way we'd have complete control over LIR_comment output, i.e. one could 'micro-comment' each LIns if they wanted.

Comment 5

[Nicholas Nethercote [inactive]]

I didn't take Rick's suggestion because I wanted to keep things as simple as possible, and there wasn't a clear use-case for excluding the "---" part.

I looked into making LIR_comment only defined if NJ_VERBOSE was defined, but things got complicated.  There is DEBUG, and NJ_VERBOSE, and TM has something called JS_JIT_SPEW which is similar to NJ_VERBOSE (ie. it can be turned on without turning on DEBUG).  I didn't want to wrangle with this.  If someone else does, please feel free to file a follow-up bug.

http://hg.mozilla.org/projects/nanojit-central/rev/50bb48a9d8ce
http://hg.mozilla.org/tracemonkey/rev/090d96500e2b

Comment 6

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/f4c36675300a

Comment 7

[Rick Reitmaier]

http://hg.mozilla.org/tamarin-redux/rev/a65ee2b02fd5

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f4c36675300a