[Webkit-unassigned] [Bug 67460] Add JSVALUE32_64 support to DFG JIT

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67460





--- Comment #19 from Yuqiang Xian <yuqiang.xian at intel.com>  2011-09-23 21:00:15 PST ---
(In reply to comment #16)
> (From update of attachment 108452 [details])
> 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.

Yes. Unnecessary code duplications need to be resolved in the near future. 

> 
> > 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?

Done.

> 
> > 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 ?: .

Done.

> 
> > 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.

Done.

> 
> > 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).

Yes, we can address this later.

> 
> > 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.

Done.

> 
> > 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?

Agree. We can address this later.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.