[Webkit-unassigned] [Bug 63858] DFG JIT does not implement op_call

[bugzilla-daemon at webkit.org]

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





--- Comment #16 from Filip Pizlo <fpizlo at apple.com>  2011-07-03 23:30:21 PST ---
> > Source/JavaScriptCore/assembler/CodeLocation.h:122
> > +class CodeLocationPossiblyNearCall : public CodeLocationCommon {
> 
> I'm not sure this extra type really adds any value.  If you want to change the type of the location held in the CodeBlock, you could just make it a CodeLocationLabel (after all, this is just a pointer into the instruction stream).  It seems highly unlikely this class will provide much value going forwards, since (1) we're likely to deprecate the old JIT, and (2) the new JIT is likely want to make near calls for JIT->JIT calls.  Still, I don't feel strongly about this - if it becomes redundant, it can be removed then.  So I'd recommend removing, but feel free to leave as is if you prefer.

Agree.

> 
> > Source/JavaScriptCore/dfg/DFGOperations.cpp:391
> > +EncodedJSValue returnValueHack();
> 
> We don't like names like like with 'hack' in them - it doesn't convey any helpful information.  I'd suggest 'getHostCallReturnValue' would be a more descriptive function name here.

Agree.

> 
> > Source/JavaScriptCore/interpreter/CallFrame.h:41
> > +        JSValue calleeAsValue() const { return this[RegisterFile::Callee].jsValue(); }
> 
> I don't think this method should be needed; you should just be able to call 'callee()', and assign the result to a JSValue.  Furthermore, this may be incorrect - I'm not sure that we write the tag value for the callee on JVALUE32_64, we may just write the JSCell*, so reading the full 64-bit entry may result in an incorrect tag being read.  Did using 'callee()' cause any problems?

I hadn't tried using callee().  The reason why I added this method is because the emitCall() path in DFGJITCodeGenerator.cpp never tests whether the target is a JSCell, and even the Speculative JIT does not speculate that it is a cell.  So at the point where we end up in DFGOperations, we could be dealing with an integer for all we know.

emitCall() currently always writes the JSValue - not the cell pointer - into the call frame.  Hence, logically, I wanted a method that returns the JSValue.  And on JSVALUE32_64, I imagine that we'd want to emit code like:

if tag != cell goto slow // unpatched compare/branch
if value != expected goto slow // patched compare, unpatched branch
perform fast call
goto done
slow:
write tag and value to call frame
call DFGOperations slow path
call *returnValueGPR
done:

The nice thing about this is that we have a common slow path for both the not-cell case and the not-expected-function case.

So, point is, my plan was that for both 32-bit and 64-bit platforms, I wanted the DFGOperations slow path to see both tag and payload.  Hence the calleeAsValue() method.  I certainly agree that this method makes no sense for the old JIT.  But in the DFG, the way I've currently implemented calls in this patch, it makes logical sense, to me at least.

Now, there's a separate question of whether this is optimal.  And whether or not it matters at all, since we're only targeting 64-bit.  For polymorphic calls where inline caching buys nothing, one could imagine that having a separate slow path for not-cell is better.

To make a long story short, I'm happy to make this change that you're recommending, though it would be a bit awkward given that callee() would be returning a JSCell even though we haven't validated that it's a JSCell.  I'm sure we could make that work on 64-bit.  Or I could change emitCall to have a separate slow path for not-cell.

So let me know if you buy the intuition for calleeAsValue(), or if you still recommend removing it.

> 
> > Source/JavaScriptCore/runtime/JSFunction.h:40
> > +    }
> 
> I don't think we normally doubly indent nested namespace declarations, this should probably all lose an indent level.

Got it.  Will change.

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