Bug 104807

Summary: Adds support for fromCharCode intrinsic
Product: WebKit Reporter: Vahag <vaag>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: barraclough, commit-queue, fpizlo, ggaren, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Adds Support for fromCharCode intrinsic
fpizlo: review-, oliver: commit-queue-
Adds fromCharCode support.
webkit-ews: commit-queue-
Adds Support for fromCharCode intrinsic
webkit-ews: commit-queue-
Switch to using fromCharCode intrinsic instead of call operation in some cases.
oliver: review+, webkit.review.bot: commit-queue-
Switch to using fromCharCode intrinsic instead of call operation in some cases.
webkit.review.bot: commit-queue-
Patch updated for latest trunk
fpizlo: review-, fpizlo: commit-queue-
Adds Support for fromCharCode intrinsic
fpizlo: review-, fpizlo: commit-queue-
FromCharCode Fixed patch
none
FromCharCode Fixed patch
fpizlo: review+, commit-queue: commit-queue-
Fixed commit-queue problem none

Description Vahag 2012-12-12 07:28:31 PST
Overview:

    Adds support for fromCharCode intrinsic

Actual Results:

   Improve performance for string operations
Comment 1 Vahag 2012-12-12 07:32:32 PST
Created attachment 179038 [details]
Adds Support for fromCharCode intrinsic
Comment 2 Vahag 2012-12-12 07:35:36 PST
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 @@
>+2012-12-12  Vahag Vardanyan  <vaag@ispras.ru>
>+
>+        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  <gaborb@inf.u-szeged.hu>
> 
>         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);
Comment 3 Oliver Hunt 2012-12-12 11:09:23 PST
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.
Comment 4 Filip Pizlo 2012-12-12 11:15:57 PST
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.
Comment 5 Filip Pizlo 2012-12-12 11:16:45 PST
(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
Comment 6 Vahag 2012-12-18 13:11:15 PST
Created attachment 180011 [details]
Adds fromCharCode support. 

New version switch to use slow path invocation.
Comment 7 Early Warning System Bot 2012-12-18 13:16:37 PST
Comment on attachment 180011 [details]
Adds fromCharCode support. 

Attachment 180011 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15408290
Comment 8 Early Warning System Bot 2012-12-18 13:17:54 PST
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
Comment 9 Vahag 2012-12-25 05:35:26 PST
Created attachment 180709 [details]
Adds Support for fromCharCode intrinsic

Fixes previous versions building problem.
Comment 10 Early Warning System Bot 2012-12-25 05:58:05 PST
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
Comment 11 Early Warning System Bot 2012-12-25 05:58:14 PST
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
Comment 12 Vahag 2013-01-07 10:44:06 PST
Created attachment 181527 [details]
Switch to using fromCharCode intrinsic instead of call operation in some cases.
Comment 13 WebKit Review Bot 2013-02-11 10:28:58 PST
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
Comment 14 Vahag 2013-02-21 05:11:01 PST
Created attachment 189511 [details]
Switch to using fromCharCode intrinsic instead of call operation in some cases.

Patch updated for latest trunk
Comment 15 WebKit Review Bot 2013-02-21 07:37:00 PST
Comment on attachment 189511 [details]
Switch to using fromCharCode intrinsic instead of call operation in some cases.

Rejecting attachment 189511 [details] from commit-queue.

vaag@ispras.ru 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.
Comment 16 Vahag 2013-04-08 13:31:32 PDT
Created attachment 196949 [details]
Patch updated for latest trunk

Patch updated for latest trunk, please review it again.
Comment 17 Filip Pizlo 2013-04-08 14:07:26 PDT
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?
Comment 18 Vahag 2013-04-08 14:36:59 PDT
Created attachment 196957 [details]
Adds Support for fromCharCode intrinsic
Comment 19 Vahag 2013-04-08 14:38:06 PDT
(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
Comment 20 Geoffrey Garen 2013-04-08 14:40:42 PDT
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.
Comment 21 Geoffrey Garen 2013-04-08 14:41:17 PDT
(The rest looks good to me.)
Comment 22 Filip Pizlo 2013-04-08 14:45:25 PDT
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.
Comment 23 Vahag 2013-04-08 14:54:36 PDT
(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.
Comment 24 Vahag 2013-04-08 14:56:18 PDT
(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
Comment 25 Filip Pizlo 2013-04-08 14:57:07 PDT
(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
Comment 26 Filip Pizlo 2013-04-08 14:57:48 PDT
(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.
Comment 27 Vahag 2013-04-08 15:23:10 PDT
(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
Comment 28 Filip Pizlo 2013-04-08 15:29:59 PDT
(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
Comment 29 Vahag 2013-04-08 15:47:50 PDT
Created attachment 196964 [details]
FromCharCode Fixed patch
Comment 30 Filip Pizlo 2013-04-08 16:06:21 PDT
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.
Comment 31 Vahag 2013-04-08 16:16:34 PDT
Created attachment 196968 [details]
FromCharCode Fixed patch
Comment 32 WebKit Commit Bot 2013-04-08 16:30:51 PDT
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
Comment 33 Vahag 2013-04-08 23:16:39 PDT
Created attachment 197001 [details]
Fixed commit-queue problem
Comment 34 WebKit Commit Bot 2013-04-08 23:43:54 PDT
Comment on attachment 197001 [details]
Fixed commit-queue problem

Clearing flags on attachment: 197001

Committed r147985: <http://trac.webkit.org/changeset/147985>
Comment 35 Csaba Osztrogon√°c 2014-02-13 04:04:07 PST
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.