[webkit-reviews] review granted: [Bug 67460] Add JSVALUE32_64 support to DFG JIT : [Attachment 108452] initial patch for JSVALUE32_64 support in DFG JIT

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has granted  review:
Bug 67460: Add JSVALUE32_64 support to DFG JIT
https://bugs.webkit.org/show_bug.cgi?id=67460

Attachment 108452: initial patch for JSVALUE32_64 support in DFG JIT
https://bugs.webkit.org/attachment.cgi?id=108452&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=108452&action=review


The biggest problem with this patch is the amount of code duplicated, but let's
get this into the tree so we can work together on better sharing code with
JSVALUE64.  There are a bunch of issues I've raised above, but they are all
things I'm happy to see addressed in follow on patches.  Otherwise all looks
good, the code looks clean, let's get this landed.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:-123
> -	   if (spillMe != InvalidVirtualRegister)

These extra loops over the gprs are a little unfortunate.  We could probably
ask the GenerationInfo if it has a paired register?

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:259
> +	   if (!info.needsSpill() || (!(info.registerFormat() & DataFormatJS)
&& matchAny(info.gpr(), info.gpr(), exclude, exclude2)) ||
((info.registerFormat() & DataFormatJS) && matchAny(info.tagGPR(),
info.payloadGPR(), exclude, exclude2)))

I think we should break:
    (!(info.registerFormat() & DataFormatJS) && matchAny(info.gpr(),
info.gpr(), exclude, exclude2)) || ((info.registerFormat() & DataFormatJS) &&
matchAny(info.tagGPR(), info.payloadGPR(), exclude, exclude2))
out into a separate helper function (since I think the same check is used again
below).  Also I think this could be written cleanly with a ?: .

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:1145
> +#elif USE(JSVALUE32_64)

This should probably really be CPU(X86), other 32_64 platforms will probably
pass arguments in registers.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.h:1157
> +	  
m_jit.push(JITCompiler::TrustedImm32(reinterpret_cast<int>(pointer)));

We probably don't want to be pushing anything in these methods.  In the JIT I
think we leave a buffer of stack space preallocated to write arguments out to
for calls out from JIT code, and differing numbers of pushes will cause
misalignment of the stack (Mac OS X requires the stack be aligned to  a 16 byte
boundary prior to the call instruction).

> Source/JavaScriptCore/dfg/DFGJITCompilerInlineMethods.h:160
> +// FIXME : Should we also add this map feature to DFG JIT?

No, I don't think so - this was a simple 1/2 entry register allocation from the
old JIT.  The DFG JIT already has more comprehensive register allocation which
can be tailored to our needs, this should probably just be removed.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:263
> +#if CPU(X86_64)

This ordering inconsistency is unfortunate, we should try to fix this at some
point.	I think the JIT code has always left sufficient space free on the stack
that we can just store above the existing arguments without tramping anything?