Bug 89551 – DFG should be able to print disassembly interleaved with the IR

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Here's a sample:



Generated JIT code for DFG CodeBlock 0x109905400:
    Code at [0x3ab1c260d960, 0x3ab1c260db2e):
          0x3ab1c260d960: pop %rcx
          0x3ab1c260d961: mov %rcx, -0x10(%r13)
          0x3ab1c260d965: mov 0x109905400, %r11
          0x3ab1c260d96f: mov %r11, -0x8(%r13)
          0x3ab1c260d973: lea 0x18(%r13), %rdx
          0x3ab1c260d977: mov 0x10986cea0, %r11
          0x3ab1c260d981: cmp %rdx, (%r11)
          0x3ab1c260d984: jb 0x3ab1c260da2d
          0x3ab1c260d98a: mov 0x109905e54, %r11
          0x3ab1c260d994: add 0x1, (%r11)
    Block #0 (bc#0): 
      Predecessors:
      Phi Nodes:
       0:           < 3:->	SetArgument(, arg0(F<Final>))  predicting Final, double ratio 0.000000
       1:           < 3:1>	GetLocal(@0, JS, arg0(F<Final>))  predicting Final, double ratio 0.000000
          0x3ab1c260d998: mov -0x38(%r13), %rax
       2:           <!0:->	CheckStructure(@1<Final>, MustGen|CanExit, struct(0x10a2b7c00))
          0x3ab1c260d99c: test %rax, %r15
          0x3ab1c260d99f: jnz 0x3ab1c260da94
          0x3ab1c260d9a5: mov 0x10a2b7c00, %r11
          0x3ab1c260d9af: cmp %r11, 0x8(%rax)
          0x3ab1c260d9b3: jnz 0x3ab1c260daaa
       4:           < 1:1>	GetByOffset(@1<Final>, JS, id0{direction}, 7)  predicting Int
          0x3ab1c260d9b9: mov 0x38(%rax), %rax
       6:           <!2:2>	GetGlobalVar(JS|MustGen, global20(0x1098bcf38))  predicting Final
          0x3ab1c260d9bd: mov 0x1098bcf38, %rdx
          0x3ab1c260d9c7: mov (%rdx), %rdx
       8:           <!0:->	CheckStructure(@6<Final>, MustGen|CanExit, struct(0x10a2b9960))
          0x3ab1c260d9ca: test %rdx, %r15
          0x3ab1c260d9cd: jnz 0x3ab1c260dac0
          0x3ab1c260d9d3: mov 0x10a2b9960, %r11
          0x3ab1c260d9dd: cmp %r11, 0x8(%rdx)
          0x3ab1c260d9e1: jnz 0x3ab1c260dad6
       9:           < 1:2>	GetByOffset(@6<Final>, JS, id1{FORWARD}, 5)  predicting Int
          0x3ab1c260d9e7: mov 0x28(%rdx), %rdx
      11:           <!1:2>	CompareEq(@4<Int32>, @9<Int32>, Boolean|MustGen|MightClobber|CanExit)
          0x3ab1c260d9eb: cmp %r14, %rax
          0x3ab1c260d9ee: jb 0x3ab1c260daec
          0x3ab1c260d9f4: cmp %r14, %rdx
          0x3ab1c260d9f7: jb 0x3ab1c260db02
          0x3ab1c260d9fd: cmp %edx, %eax
          0x3ab1c260d9ff: jnz 0x3ab1c260da16
    Block #1 (bc#33): 
      Predecessors: #0
      Phi Nodes: @14->(@0)
      15:           < 2:2>	GetLocal(@14, JS, arg0(F<Final>))  predicting Final, double ratio 0.000000
          0x3ab1c260da05: mov -0x38(%r13), %rcx
      16:           <!0:->	CheckStructure(@15<Final>, MustGen, struct(0x10a2b7c00))
      17:           < 1:2>	GetByOffset(@15<Final>, JS, id2{v2}, 6)  predicting Final
          0x3ab1c260da09: mov 0x30(%rcx), %rcx
      18:           < 1:->	SetLocal(@17<Final>, , r0(J<Final>))  predicting Final, double ratio 0.000000
          0x3ab1c260da0d: mov %rcx, 0x0(%r13)
      19:           <!0:->	Jump(MustGen, T:#3)
          0x3ab1c260da11: jmp 0x3ab1c260da1f
    Block #2 (bc#44): 
      Predecessors: #0
      Phi Nodes: @20->(@0)
      21:           < 1:2>	GetLocal(@20, JS, arg0(F<Final>))  predicting Final, double ratio 0.000000
          0x3ab1c260da16: mov -0x38(%r13), %rbx
      22:           <!1:2>	GetById(@21<Final>, JS|MustGen|Clobbers|CanExit, id3{v1})  predicting None
          0x3ab1c260da1a: jmp 0x3ab1c260db18
    Block #3 (bc#53): 
      Predecessors: #2 #1
      Phi Nodes: @25->(@23, @18)
      26:           < 1:2>	GetLocal(@25, JS, r0(J<Final>))  predicting Final, double ratio 0.000000
          0x3ab1c260da1f: mov 0x0(%r13), %rax
      27:           <!0:->	Return(@26<Final>, MustGen)
          0x3ab1c260da23: mov -0x10(%r13), %rdx
          0x3ab1c260da27: mov -0x28(%r13), %r13
          0x3ab1c260da2b: push %rdx
          0x3ab1c260da2c: ret 
    (End Of Main Path)
          0x3ab1c260da2d: mov %rsp, %rdi
          0x3ab1c260da30: mov %r13, 0x58(%rsp)
          0x3ab1c260da35: mov 0x0, -0x2c(%r13)
          0x3ab1c260da3d: mov 0x10943e4c0, %r11
          0x3ab1c260da47: call %r11
          0x3ab1c260da4a: jmp 0x3ab1c260d98a
          0x3ab1c260da4f: pop %rcx
          0x3ab1c260da50: mov %rcx, -0x10(%r13)
          0x3ab1c260da54: mov 0x109905400, %r11
          0x3ab1c260da5e: mov %r11, -0x8(%r13)
          0x3ab1c260da62: mov -0x30(%r13), %edx
          0x3ab1c260da66: cmp 0x1, %edx
          0x3ab1c260da69: jae 0x3ab1c260d973
          0x3ab1c260da6f: mov %rsp, %rdi
          0x3ab1c260da72: mov %r13, 0x58(%rsp)
          0x3ab1c260da77: mov 0x1, -0x2c(%r13)
          0x3ab1c260da7f: mov 0x109440650, %r11
          0x3ab1c260da89: call %r11
          0x3ab1c260da8c: mov %rax, %r13
          0x3ab1c260da8f: jmp 0x3ab1c260d973
          0x3ab1c260da94: mov 0x109859828, %r11
          0x3ab1c260da9e: mov 0x0, (%r11)
          0x3ab1c260daa5: jmp 0x3ab1c260db40
          0x3ab1c260daaa: mov 0x109859828, %r11
          0x3ab1c260dab4: mov 0x1, (%r11)
          0x3ab1c260dabb: jmp 0x3ab1c260db40
          0x3ab1c260dac0: mov 0x109859828, %r11
          0x3ab1c260daca: mov 0x2, (%r11)
          0x3ab1c260dad1: jmp 0x3ab1c260db40
          0x3ab1c260dad6: mov 0x109859828, %r11
          0x3ab1c260dae0: mov 0x3, (%r11)
          0x3ab1c260dae7: jmp 0x3ab1c260db40
          0x3ab1c260daec: mov 0x109859828, %r11
          0x3ab1c260daf6: mov 0x4, (%r11)
          0x3ab1c260dafd: jmp 0x3ab1c260db40
          0x3ab1c260db02: mov 0x109859828, %r11
          0x3ab1c260db0c: mov 0x5, (%r11)
          0x3ab1c260db13: jmp 0x3ab1c260db40
          0x3ab1c260db18: mov 0x109859828, %r11
          0x3ab1c260db22: mov 0x6, (%r11)
          0x3ab1c260db29: jmp 0x3ab1c260db40

[Filip Pizlo]

Created attachment 148517 [details]
the patch

[WebKit Review Bot]

Attachment 148517 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/dfg/DFGGraph.h:185:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/dfg/DFGGraph.h:186:  The parameter name "node" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 148519 [details]
improved patch

Fixed style issues.

Also fixed a small issue in dumping of code that was not generated. The previous patch would dump neither the IR nor the disassembly for such nodes. The current patch will dump the IR but not the disassembly, and will make sure that the disassembly for the previous node is dumped correctly.  To be clear, what I mean by "code that was not generated" is either code that was after a forced OSR exit (the IR for such code is relevant from the standpoint of liveness) or Branches that were not really emitted due to peephole optimizations.

[Geoffrey Garen]

Comment on attachment 148519 [details]
improved patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148519&action=review

r=me, but please double-check dominators removal, and add a comment to the ChangeLog explaining why.

> Source/JavaScriptCore/dfg/DFGDriver.cpp:-95
> -    dfg.m_dominators.compute(dfg);

Was this on purpose? Seems wrong.

[Filip Pizlo]

(In reply to comment #5)
> (From update of attachment 148519 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148519&action=review
> 
> r=me, but please double-check dominators removal, and add a comment to the ChangeLog explaining why.
> 
> > Source/JavaScriptCore/dfg/DFGDriver.cpp:-95
> > -    dfg.m_dominators.compute(dfg);
> 
> Was this on purpose? Seems wrong.

It was, but then I forgot to finish the job.

We have a couple of things in the DFG code-base that were done as first steps to SSA but that we never used.  They should either get killed or find new life.

One is the redundant Phi elimination phase, which is just now wrong.  The code is sitting there but isn't called from anywhere.

The other is dominators.  I thought I'd use them for arguments simplification, but then I decided not to.  But, in the meantime, I found dominators to be useful for IR dumps.  They make it easier for me to figure out block relationships.  So, what this patch was supposed to do, is remove unconditional dominator computation, and instead call Dominators::computeIfNecessary() inside of Disassembler.

I'll land with that change.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/120834

[Filip Pizlo]

Forgot to add the ChangeLog comment, added in http://trac.webkit.org/changeset/120836