Bug 104807 – Adds support for fromCharCode intrinsic

• Status: UNCONFIRMED
• Component: JavaScriptCore

[Vahag]

Overview:

    Adds support for fromCharCode intrinsic

Actual Results:

   Improve performance for string operations

[Vahag]

Created attachment 179038 [details]
Adds Support for fromCharCode intrinsic

[Vahag]

Comment on attachment 179038 [details]
Adds Support for fromCharCode intrinsic

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 137464)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,30 @@
>[phone]  Vahag Vardanyan  <[e-mail]>
>+
>+        Adds support for fromCharCode intrinsic
>+        https://bugs.webkit.org/show_bug.cgi?id=104807
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
>+
>+        * dfg/DFGAbstractState.cpp:
>+        (JSC::DFG::AbstractState::execute):
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::handleIntrinsic):
>+        * dfg/DFGNodeType.h:
>+        (DFG):
>+        * dfg/DFGPredictionPropagationPhase.cpp:
>+        (JSC::DFG::PredictionPropagationPhase::propagate):
>+        * dfg/DFGSpeculativeJIT.cpp:
>+        (JSC::DFG::SpeculativeJIT::compileFromCharCode):
>+        (DFG):
>+        * dfg/DFGSpeculativeJIT.h:
>+        (SpeculativeJIT):
>+        * dfg/DFGSpeculativeJIT32_64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * dfg/DFGSpeculativeJIT64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+
> 2012-12-11  Gabor Ballabas  <[e-mail]>
> 
>         Implement add64 for ARM traditional assembler after r136601
>Index: Source/JavaScriptCore/dfg/DFGNodeType.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGNodeType.h	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGNodeType.h	(working copy)
>@@ -174,6 +174,7 @@
>     /* Optimizations for string access */ \
>     macro(StringCharCodeAt, NodeResultInt32) \
>     macro(StringCharAt, NodeResultJS) \
>+    macro(StringFromCharCode, NodeResultJS) \
>     \
>     /* Nodes for comparison operations. */\
>     macro(CompareLess, NodeResultBoolean | NodeMustGenerate | NodeMightClobber) \
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(working copy)
>@@ -2582,6 +2582,11 @@
>         compileGetByValOnString(node);
>         break;
>     }
>+
>+    case StringFromCharCode: {
>+        compileFromCharCode(node);
>+        break;
>+    }
>         
>     case CheckArray: {
>         checkArray(node);
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(working copy)
>@@ -2551,6 +2551,11 @@
>         compileGetByValOnString(node);
>         break;
>     }
>+
>+    case StringFromCharCode: {
>+        compileFromCharCode(node);
>+        break;
>+    }
>         
>     case CheckArray: {
>         checkArray(node);
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(working copy)
>@@ -2016,6 +2016,25 @@
>     cellResult(scratchReg, m_compileIndex);
> }
> 
>+void SpeculativeJIT::compileFromCharCode(Node& node)
>+{
>+    SpeculateStrictInt32Operand property(this, node.child1());
>+    GPRReg propertyReg = property.gpr();
>+    GPRTemporary smallStrings(this);
>+    GPRTemporary scratch(this);
>+    GPRReg scratchReg = scratch.gpr();
>+    GPRReg smallStringsReg = smallStrings.gpr();
>+
>+    // Only ASCII characters are supported
>+    speculationCheck(Uncountable, JSValueRegs(), NoNode, m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, TrustedImm32(0x100)));
>+
>+    m_jit.move(MacroAssembler::TrustedImmPtr(m_jit.globalData()->smallStrings.singleCharacterStrings()), smallStringsReg);
>+    m_jit.loadPtr(MacroAssembler::BaseIndex(smallStringsReg, propertyReg, MacroAssembler::ScalePtr, 0), scratchReg);
>+    speculationCheck(Uncountable, JSValueRegs(), NoNode, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));
>+
>+    cellResult(scratchReg, m_compileIndex);
>+}
>+
> GeneratedOperandType SpeculativeJIT::checkGeneratedTypeForToInt32(NodeIndex nodeIndex)
> {
> #if DFG_ENABLE(DEBUG_VERBOSE)
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h	(working copy)
>@@ -2338,6 +2338,7 @@
>     
>     void compileGetCharCodeAt(Node&);
>     void compileGetByValOnString(Node&);
>+    void compileFromCharCode(Node&);
> 
>     void compileGetByValOnArguments(Node&);
>     void compileGetArgumentsLength(Node&);
>Index: Source/JavaScriptCore/dfg/DFGAbstractState.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGAbstractState.cpp	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGAbstractState.cpp	(working copy)
>@@ -887,6 +887,12 @@
>         forNode(nodeIndex).set(SpecInt32);
>         break;
>         
>+    case StringFromCharCode:
>+        node.setCanExit(true);
>+        forNode(node.child1()).filter(SpecInt32);
>+        forNode(nodeIndex).set(SpecString);
>+        break;
>+
>     case StringCharAt:
>         node.setCanExit(true);
>         forNode(node.child1()).filter(SpecString);
>Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -1734,7 +1734,19 @@
>             set(resultOperand, charCode);
>         return true;
>     }
>+    case FromCharCodeIntrinsic: {
>+        if (argumentCountIncludingThis != 2)
>+            return false;
> 
>+        int indexOperand = registerOffset + argumentToOperand(1);
>+        NodeIndex charCode = addToGraph(StringFromCharCode, getToInt32(indexOperand));
>+
>+        if (usesResult)
>+            set(resultOperand, charCode);
>+
>+        return true;
>+    }
>+
>     case RegExpExecIntrinsic: {
>         if (argumentCountIncludingThis != 2)
>             return false;
>Index: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(revision 137458)
>+++ Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(working copy)
>@@ -640,6 +640,12 @@
>             break;
>         }
>         
>+        case StringFromCharCode: {
>+            changed |= setPrediction(SpecString);
>+            changed |= m_graph[node.child1()].mergeFlags(NodeUsedAsNumber | NodeUsedAsInt);
>+            break;
>+        }
>+
>         case StringCharAt: {
>             changed |= setPrediction(SpecString);
>             changed |= m_graph[node.child1()].mergeFlags(NodeUsedAsValue);

[Oliver Hunt]

Comment on attachment 179038 [details]
Adds Support for fromCharCode intrinsic

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

Code looks good to me, but you need to make sure you have a real change log.  Once you've got that re-upload and i'll re-review and land.

> Source/JavaScriptCore/ChangeLog:8
> +        Need a short description (OOPS!).
> +        https://bugs.webkit.org/show_bug.cgi?id=104807
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Alas you need a real change log.

[Filip Pizlo]

Comment on attachment 179038 [details]
Adds Support for fromCharCode intrinsic

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2029
> +    speculationCheck(Uncountable, JSValueRegs(), NoNode, m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, TrustedImm32(0x100)));

I know that the other string intrinsically do this as well but I would rather we not perpetuate it. Your patch could easily be a huge regression if someone does a Unicode fromcharcodeat that dominates a look. Please switch this to a slow path invocation.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 179038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179038&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2029
> > +    speculationCheck(Uncountable, JSValueRegs(), NoNode, m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, TrustedImm32(0x100)));
> 
> I know that the other string intrinsically do this as well but I would rather we not perpetuate it. Your patch could easily be a huge regression if someone does a Unicode fromcharcodeat that dominates a look. Please switch this to a slow path invocation.

*that dominates a loop. 

-F

[Vahag]

Created attachment 180011 [details]
Adds fromCharCode support. 

New version switch to use slow path invocation.

[Early Warning System Bot]

Comment on attachment 180011 [details]
Adds fromCharCode support. 

Attachment 180011 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15408290

[Early Warning System Bot]

Comment on attachment 180011 [details]
Adds fromCharCode support. 

Attachment 180011 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15405303

[Vahag]

Created attachment 180709 [details]
Adds Support for fromCharCode intrinsic

Fixes previous versions building problem.

[Early Warning System Bot]

Comment on attachment 180709 [details]
Adds Support for fromCharCode intrinsic

Attachment 180709 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15492844

[Early Warning System Bot]

Comment on attachment 180709 [details]
Adds Support for fromCharCode intrinsic

Attachment 180709 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15491866

[Vahag]

Created attachment 181527 [details]
Switch to using fromCharCode intrinsic instead of call operation in some cases.

[WebKit Review Bot]

Comment on attachment 181527 [details]
Switch to using fromCharCode intrinsic instead of call operation in some cases.

Rejecting attachment 181527 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 181527, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
rej
patching file Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Hunk #1 succeeded at 1587 (offset -114 lines).
patching file Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Hunk #1 succeeded at 665 with fuzz 1 (offset 4 lines).
patching file Source/JavaScriptCore/dfg/DFGNodeType.h
Hunk #1 succeeded at 179 (offset 1 line).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Oliver Hunt']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16491312

[Vahag]

Created attachment 189511 [details]
Switch to using fromCharCode intrinsic instead of call operation in some cases.

Patch updated for latest trunk

[WebKit Review Bot]

Comment on attachment 189511 [details]
Switch to using fromCharCode intrinsic instead of call operation in some cases.

Rejecting attachment 189511 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Vahag]

Created attachment 196949 [details]
Patch updated for latest trunk

Patch updated for latest trunk, please review it again.

[Filip Pizlo]

Comment on attachment 196949 [details]
Patch updated for latest trunk

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2109
> +    JITCompiler::Jump jmpAfter = m_jit.jump();
> +
> +    isNotASCIICharacter.link(&m_jit);
> +    JITCompiler::Jump jmpSlowCase = m_jit.jump();
> +    addSlowPathGenerator(slowPathCall(jmpSlowCase, this, operationStringFromCharCode, scratchReg, propertyReg));
> +
> +    jmpAfter.link(&m_jit);

This is wrong.  You should be passing isNotASCIICharacter as the first argument to slowPathCall(), and omitting the jmpAfter thing.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2110
> +    speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));

Why?

[Vahag]

Created attachment 196957 [details]
Adds Support for fromCharCode intrinsic

[Vahag]

(In reply to comment #17)
> (From update of attachment 196949 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196949&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2109
> > +    JITCompiler::Jump jmpAfter = m_jit.jump();
> > +
> > +    isNotASCIICharacter.link(&m_jit);
> > +    JITCompiler::Jump jmpSlowCase = m_jit.jump();
> > +    addSlowPathGenerator(slowPathCall(jmpSlowCase, this, operationStringFromCharCode, scratchReg, propertyReg));
> > +
> > +    jmpAfter.link(&m_jit);
> 
> This is wrong.  You should be passing isNotASCIICharacter as the first argument to slowPathCall(), and omitting the jmpAfter thing.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2110
> > +    speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));
> 
> Why?

Fixed

[Geoffrey Garen]

Comment on attachment 196957 [details]
Adds Support for fromCharCode intrinsic 

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2100
> +    JITCompiler::Jump isNotASCIICharacter = m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, TrustedImm32(0x100));

Can we test for above 0xff here instead? I believe that's a slightly smaller encoded immediate on most platforms.

[Geoffrey Garen]

(The rest looks good to me.)

[Filip Pizlo]

Comment on attachment 196957 [details]
Adds Support for fromCharCode intrinsic 

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

LGTM except for those minor fixes.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:865
> +        node->setCanExit(true);

Not needed.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:866
> +        forNode(node->child1()).filter(SpecInt32);

Not needed.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:352
> +            setUseKindAndUnboxIfProfitable<Int32Use>(node->child1());

The reason why you don't need the filter() and setCanExit() in AbstractState is that AbstractState::executeEdges() takes care of this based on Int32Use.

[Vahag]

(In reply to comment #22)
> (From update of attachment 196957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196957&action=review
> 
> LGTM except for those minor fixes.
> 
> > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:865
> > +        node->setCanExit(true);
> 
> Not needed.
> 
> > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:866
> > +        forNode(node->child1()).filter(SpecInt32);
> 
> Not needed.
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:352
> > +            setUseKindAndUnboxIfProfitable<Int32Use>(node->child1());
It will fail on SpeculateStrictInt32Operand property(this, node->child1()); without this.

> 
> The reason why you don't need the filter() and setCanExit() in AbstractState is that AbstractState::executeEdges() takes care of this based on Int32Use.

[Vahag]

(In reply to comment #19)
> (In reply to comment #17)
> > (From update of attachment 196949 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=196949&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2109
> > > +    JITCompiler::Jump jmpAfter = m_jit.jump();
> > > +
> > > +    isNotASCIICharacter.link(&m_jit);
> > > +    JITCompiler::Jump jmpSlowCase = m_jit.jump();
> > > +    addSlowPathGenerator(slowPathCall(jmpSlowCase, this, operationStringFromCharCode, scratchReg, propertyReg));
> > > +
> > > +    jmpAfter.link(&m_jit);
> > 
> > This is wrong.  You should be passing isNotASCIICharacter as the first argument to slowPathCall(), and omitting the jmpAfter thing.
> > 
> > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2110
> > > +    speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));
> > 
> > Why?
> 
It fails on some tests without this check, for example on stringSHA1 from browsemark.
> Fixed

[Filip Pizlo]

(In reply to comment #24)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (From update of attachment 196949 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=196949&action=review
> > > 
> > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2109
> > > > +    JITCompiler::Jump jmpAfter = m_jit.jump();
> > > > +
> > > > +    isNotASCIICharacter.link(&m_jit);
> > > > +    JITCompiler::Jump jmpSlowCase = m_jit.jump();
> > > > +    addSlowPathGenerator(slowPathCall(jmpSlowCase, this, operationStringFromCharCode, scratchReg, propertyReg));
> > > > +
> > > > +    jmpAfter.link(&m_jit);
> > > 
> > > This is wrong.  You should be passing isNotASCIICharacter as the first argument to slowPathCall(), and omitting the jmpAfter thing.
> > > 
> > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2110
> > > > +    speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));
> > > 
> > > Why?
> > 
> It fails on some tests without this check, for example on stringSHA1 from browsemark.

OK, but why?

> > Fixed

[Filip Pizlo]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 196957 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=196957&action=review
> > 
> > LGTM except for those minor fixes.
> > 
> > > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:865
> > > +        node->setCanExit(true);
> > 
> > Not needed.
> > 
> > > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:866
> > > +        forNode(node->child1()).filter(SpecInt32);
> > 
> > Not needed.
> > 
> > > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:352
> > > +            setUseKindAndUnboxIfProfitable<Int32Use>(node->child1());
> It will fail on SpeculateStrictInt32Operand property(this, node->child1()); without this.
> 

This line that sets the use kind *is* needed.  I'm not saying that it isn't.  I'm saying that the setCanExit() and filter(SpecInt32) in AbstractState aren't needed.

> > 
> > The reason why you don't need the filter() and setCanExit() in AbstractState is that AbstractState::executeEdges() takes care of this based on Int32Use.

[Vahag]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #19)
> > > (In reply to comment #17)
> > > > (From update of attachment 196949 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=196949&action=review
> > > > 
> > > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2109
> > > > > +    JITCompiler::Jump jmpAfter = m_jit.jump();
> > > > > +
> > > > > +    isNotASCIICharacter.link(&m_jit);
> > > > > +    JITCompiler::Jump jmpSlowCase = m_jit.jump();
> > > > > +    addSlowPathGenerator(slowPathCall(jmpSlowCase, this, operationStringFromCharCode, scratchReg, propertyReg));
> > > > > +
> > > > > +    jmpAfter.link(&m_jit);
> > > > 
> > > > This is wrong.  You should be passing isNotASCIICharacter as the first argument to slowPathCall(), and omitting the jmpAfter thing.
> > > > 
> > > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2110
> > > > > +    speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));
> > > > 
> > > > Why?
> > > 
> > It fails on some tests without this check, for example on stringSHA1 from browsemark.
> 
> OK, but why?
> Can't figure out yet, but the check is exists for StringCharAtIntrisic. 
> > > Fixed

[Filip Pizlo]

(In reply to comment #27)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (In reply to comment #19)
> > > > (In reply to comment #17)
> > > > > (From update of attachment 196949 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=196949&action=review
> > > > > 
> > > > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2109
> > > > > > +    JITCompiler::Jump jmpAfter = m_jit.jump();
> > > > > > +
> > > > > > +    isNotASCIICharacter.link(&m_jit);
> > > > > > +    JITCompiler::Jump jmpSlowCase = m_jit.jump();
> > > > > > +    addSlowPathGenerator(slowPathCall(jmpSlowCase, this, operationStringFromCharCode, scratchReg, propertyReg));
> > > > > > +
> > > > > > +    jmpAfter.link(&m_jit);
> > > > > 
> > > > > This is wrong.  You should be passing isNotASCIICharacter as the first argument to slowPathCall(), and omitting the jmpAfter thing.
> > > > > 
> > > > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2110
> > > > > > +    speculationCheck(Uncountable, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Zero, scratchReg));
> > > > > 
> > > > > Why?
> > > > 
> > > It fails on some tests without this check, for example on stringSHA1 from browsemark.
> > 
> > OK, but why?
> > Can't figure out yet, but the check is exists for StringCharAtIntrisic. 

Aha, I think I know: the string table is filled lazily and may be cleared by GC.

While speculation failure is sound - it won't cause the program to crash - it is a really inefficient way of doing things.  I understand that there may be other code in the DFG that is similarly broken but we should fix that separately and not introduce that bug here.  The problem is that we don't want to blindly speculate on properties that we know may not hold.  Imagine a program in which small strings are always transient and always cleared by GC.  Speculating means you want the code block to be recompiled because you encountered an invalid assumption (we don't recompile immediately but we will after some small number of speculation failures).  Do you believe that it is best to recompile the code block because the GC cleared a small string?  Probably, it's not what you wanted.

I think what you want to do is call to slow path if the entry is empty.  You can do it by having a slow path JumpList that includes both the not-ascii case and the null small string case.  Your current slow path will take care of both of those cases correctly, I think - so it should be only a modest change to your code in DFG::SpeculativeJIT.

> > > > Fixed

[Vahag]

Created attachment 196964 [details]
FromCharCode Fixed patch

[Filip Pizlo]

Comment on attachment 196964 [details]
FromCharCode Fixed patch

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

LGTM other than the minor fixes.  Marking r+/cq-; I'll cq+ the patch with those minor fixes.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:1593
> +

Unnecessary white space.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2100
> +    JITCompiler::JumpList isNotASCIICharacter;

Rename this to just slowCase or slowPath, since it's no longer about whether or not the input is an ASCII character.

[Vahag]

Created attachment 196968 [details]
FromCharCode Fixed patch

[WebKit Commit Bot]

Comment on attachment 196968 [details]
FromCharCode Fixed patch

Rejecting attachment 196968 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--non-interactive', 196968, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17635013

[Vahag]

Created attachment 197001 [details]
Fixed commit-queue problem

[WebKit Commit Bot]

Comment on attachment 197001 [details]
Fixed commit-queue problem

Clearing flags on attachment: 197001

Committed r147985: <http://trac.webkit.org/changeset/147985>

[Csaba Osztrogonác]

Comment on attachment 196964 [details]
FromCharCode Fixed patch

Cleared Filip Pizlo's review+ from obsolete attachment 196964 [details] so that this bug does not appear in http://webkit.org/pending-commit.