[webkit-reviews] review denied: [Bug 56284] Add a dataflow intermediate representation for use in JIT generation. : [Attachment 85651] With geoff's comments fixed (bar abstracting out the alias mechanism).

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 56284: Add a dataflow intermediate representation for use in JIT
generation.
https://bugs.webkit.org/show_bug.cgi?id=56284

Attachment 85651: With geoff's comments fixed (bar abstracting out the alias
mechanism).
https://bugs.webkit.org/attachment.cgi?id=85651&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=85651&action=review

r- due to various style issues

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:66
> +	      
m_jit.loadPtr(MacroAssembler::Address(JITCompiler::callFrameRegister, (argument
- (m_jit.codeBlock()->m_numParameters + RegisterFile::CallFrameHeaderSize)) *
sizeof(Register)), reg);

This should use addressFor(argument - (m_jit.codeBlock()->m_numParameters +
RegisterFile::CallFrameHeaderSize)) rather than explicitly calling the Address
constructor.  It's much easier to read.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:70
> +	      
m_jit.loadPtr(MacroAssembler::Address(JITCompiler::callFrameRegister,
virtualRegister * sizeof(Register)), reg);

addressFor(virtualRegister)

Using addressFor(), etc makes it much harder to screwup in the face of
different byte orders, etc.   eg. int loads/stores need to go to the right part
of the register.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:157
> +	      
m_jit.loadPtr(MacroAssembler::Address(JITCompiler::callFrameRegister,
virtualRegister * sizeof(Register)), reg);

I strongly recommend just going through this patch and looking for any use of
Address() that is based of of callFrameRegister and swith to addressFor