Bug 125470 – CStack Branch: Fix baseline JIT for basic operation

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

[Michael Saboff]

This task is to enable basic JavaScript processing in the baseline JIT engine when running on the C-stack.

[Michael Saboff]

Created attachment 218808 [details]
Patch

With this patch, we can run v8-crypto.js with the baseline JIT enabled (but with the DFG off).

[Filip Pizlo]

Comment on attachment 218808 [details]
Patch

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

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:243
> -static MacroAssemblerCodeRef nativeForGenerator(VM* vm, CodeSpecializationKind kind)
> +static MacroAssemblerCodeRef nativeForGenerator(VM* vm, CodeSpecializationKind kind, bool fallBack = false)

What's this fallback thing?

[Geoffrey Garen]

Comment on attachment 218808 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Fixed compileOpCall and it's slow case to properly adjust the stack pointer before

Should be "its".

> Source/JavaScriptCore/ChangeLog:16
> +        differes from ctiNativeCall() in that it doesn't emit a funciton prologue, thus 

Should be "function".

> Source/JavaScriptCore/ChangeLog:17
> +        allowing the original thunk to jump to the "fallback" thunk.  I'm open to another

I don't get this. The original thunk did not emit a function prologue. So, who does emit the function prologue when a thunk fails, and needs to call out to C++? Alternatively, if a thunk does not need a function prologue, why does a host function call ever need a function prologue?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:265
> +    checkStackPointerAlignment(); // &&&&

Why the &'s?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:781
> +    checkStackPointerAlignment(); // &&&&

Why the &'s?

> Source/JavaScriptCore/jit/JITThunks.cpp:65
> +MacroAssemblerCodePtr JITThunks::ctiNativeCallFallback(VM* vm)

Let's call this ctiNativeTailCall.

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:243
>> +static MacroAssemblerCodeRef nativeForGenerator(VM* vm, CodeSpecializationKind kind, bool fallBack = false)
> 
> What's this fallback thing?

Instead of "fallback", let's call this "shouldEmitFunctionPrologue", or use a named enum.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:417
> +MacroAssemblerCodeRef nativeCallFallbackGenerator(VM* vm)

Let's call this nativeTailCallGenerator.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:419
> +    return nativeForGenerator(vm, CodeForCall, true);

You should put this boolean into a named local variable, or change the function's argument to a named enum. "true" is unreadable in this context.

[Michael Saboff]

(In reply to comment #3)
> (From update of attachment 218808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218808&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Fixed compileOpCall and it's slow case to properly adjust the stack pointer before
> 
> Should be "its".

Fixed.

> > Source/JavaScriptCore/ChangeLog:16
> > +        differes from ctiNativeCall() in that it doesn't emit a funciton prologue, thus 
> 
> Should be "function".

Fixed.

> > Source/JavaScriptCore/ChangeLog:17
> > +        allowing the original thunk to jump to the "fallback" thunk.  I'm open to another
> 
> I don't get this. The original thunk did not emit a function prologue. So, who does emit the function prologue when a thunk fails, and needs to call out to C++? Alternatively, if a thunk does not need a function prologue, why does a host function call ever need a function prologue?

ctiNativeCall() has to emit a function prologue because we create a standard frame for the native call and want it to look like any other call.  In the case of a primary thunk like charCodeAt, they too emit a function prologue in the SpecializedThunkJIT constructor. When the primary thunk needs to call out to C, we could try undoing the prologue.  It seems much more straightforward to just have the first thunk jump to the second host callout thunk that doesn't have a prologue.
 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:265
> > +    checkStackPointerAlignment(); // &&&&
> 
> Why the &'s?

As I said in an earlier ChangeLog entry 'There are several places where the code is tagged with "&&&& FIXME: ..." comments as placeholders where more work needs to be done.'  This is the main way that I leave markers in the code for things need to be dealt with before I consider them done.  In this place I didn't include the FIXME as it isn't my normal practice. When working in my own sandbox, the &&&& and the issues they tag are eliminated (dealt with) before posting a patch for review.  Since this branch has my work in progress, it works best for me if they appear temporarily in the checked in code.  They will all be removed when this work is done.

> > Source/JavaScriptCore/jit/JITOpcodes.cpp:781
> > +    checkStackPointerAlignment(); // &&&&
> 
> Why the &'s?
> 
> > Source/JavaScriptCore/jit/JITThunks.cpp:65
> > +MacroAssemblerCodePtr JITThunks::ctiNativeCallFallback(VM* vm)
> 
> Let's call this ctiNativeTailCall.

Fine by me.  I'll change that as well in another patch.  Tracked in https://bugs.webkit.org/show_bug.cgi?id=125485 - "CStack Branch: ctiNativeCallFallback and friends should renamed ...NativeTailCall"
 
> >> Source/JavaScriptCore/jit/ThunkGenerators.cpp:243
> >> +static MacroAssemblerCodeRef nativeForGenerator(VM* vm, CodeSpecializationKind kind, bool fallBack = false)
> > 
> > What's this fallback thing?
> 
> Instead of "fallback", let's call this "shouldEmitFunctionPrologue", or use a named enum.

In <https://bugs.webkit.org/show_bug.cgi?id=125473> I changed it to "enum ThunkEntryType { EnterViaCall, EnterViaJump };"
 
> > Source/JavaScriptCore/jit/ThunkGenerators.cpp:417
> > +MacroAssemblerCodeRef nativeCallFallbackGenerator(VM* vm)
> 
> Let's call this nativeTailCallGenerator.

I'll change this in another patch.  Also tracked in https://bugs.webkit.org/show_bug.cgi?id=125485.

> > Source/JavaScriptCore/jit/ThunkGenerators.cpp:419
> > +    return nativeForGenerator(vm, CodeForCall, true);
> 
> You should put this boolean into a named local variable, or change the function's argument to a named enum. "true" is unreadable in this context.

See above.

[Geoffrey Garen]

Comment on attachment 218808 [details]
Patch

I think this ended up landing. Clearing review flag.

[Michael Saboff]

(In reply to comment #5)
> (From update of attachment 218808 [details])
> I think this ended up landing. Clearing review flag.

Are you going to update the ChangeLog entry or should I?

[Michael Saboff]

Fix merged back to trunk in r163027.