Bug 193240 – [ESNext][BigInt] Add support for op_inc

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

...

[Caio Lima]

Created attachment 358966 [details]
WIP - Patch

[EWS Watchlist]

Attachment 358966 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1452:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 358966 [details]
WIP - Patch

Attachment 358966 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10720373

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 358968 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Caio Lima]

Created attachment 359025 [details]
WIP - Patch

[EWS Watchlist]

Attachment 359025 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1452:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 359035 [details]
WIP - Patch

[EWS Watchlist]

Attachment 359035 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:1452:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robin Morisset]

Created attachment 382769 [details]
WIP

Still missing tests.

[Robin Morisset]

Created attachment 383081 [details]
Patch

[Robin Morisset]

Created attachment 383101 [details]
Patch

rebased now that the ArithProfile patch is back in.

[EWS Watchlist]

Comment on attachment 383101 [details]
Patch

Attachment 383101 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13225136

New failing tests:
stress/arith-negate-on-various-types.js.default
stress/arith-negate-on-various-types.js.no-cjit-collect-continuously
stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling-profiler
stress/arith-negate-on-various-types.js.no-cjit-validate-phases
stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o0
stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate
stress/arith-negate-on-various-types.js.no-ftl
stress/arith-negate-on-various-types.js.no-llint

[Caio Lima]

Comment on attachment 383101 [details]
Patch

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

Thank you very much for doing work on this. I left some comments in the Patch.

> Source/JavaScriptCore/ChangeLog:11
> +        - teaching FixupPhase how to replace it by ArithAdd/ArithSub when the type is Int32/Double/BigInt

When type is BigInt, we actually fixup this to ValueAdd/ValueSub.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:935
> +    case Dec: {

We should be able to apply constant folding when we prove that `node->child1()` is a number constant, even when `node->child1().useKind() == UntypedUse`.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:203
> +                node->setResult(NodeResultInt32);

We can also `node->clearFlags(NodeMustGenerate);` here.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:210
> +                fixEdge<BigIntUse>(node->child2());

And here.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:212
> +

nit: remove blank line. I also think the comment above is unnecessary.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:213
> +            } else {

nit: this else is not needed.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1181
> +        case Dec:

I think rules for prediction propagation are missing there.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:666
> +    auto bytecode = currentInstruction->as<OpToNumber>();

I think you want to do `OpToNumeric`.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:671
> +    // FIXME: fill in!

I don't understand this FIXME here. Can you create a bug and elaborate on that?

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:844
> +    auto bytecode = currentInstruction->as<OpToNumber>();

Ditto.

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:851
> +    SDjkadakdhsakj;

I suspect this is a compilation error.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:464
> +        result = JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(resultVariant));

Why do we use here `JSBigInt::inc`, wihle using `JSBigInt::add(..., vm.bigIntConstantOne.get())` into `operationInc` and `operationDec`?

> Source/JavaScriptCore/runtime/VM.cpp:396
> +        bigIntConstantOne.set(*this, JSBigInt::createFrom(*this, 1));

What happens in case `canUseJIT() == false`? This constant is still being used by LLInt on `JSBigInt::inc` or `JSBigInt::dec` and I suspect we will have an error in such scenario.

[Robin Morisset]

Comment on attachment 383101 [details]
Patch

I clearly missed a couple FIXME, surprising it built at all, even more that it passed tests locally. Will fix, add more tests and re-upload.
Thanks Caio for your review, will answer it in details in a few hours.

[Robin Morisset]

(In reply to Caio Lima from comment #13)
> Comment on attachment 383101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383101&action=review
> 
> Thank you very much for doing work on this. I left some comments in the
> Patch.
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        - teaching FixupPhase how to replace it by ArithAdd/ArithSub when the type is Int32/Double/BigInt
> 
> When type is BigInt, we actually fixup this to ValueAdd/ValueSub.

Fixed.

> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:935
> > +    case Dec: {
> 
> We should be able to apply constant folding when we prove that
> `node->child1()` is a number constant, even when `node->child1().useKind()
> == UntypedUse`.

I added a FIXME to look at it later. I doubt it is going to be easy to test, since fixup phase should run before, and in the simple case will replace this by an Add or Sub.

> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:203
> > +                node->setResult(NodeResultInt32);
> 
> We can also `node->clearFlags(NodeMustGenerate);` here.

It is done a few lines later, after the if/then/else.
 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:210
> > +                fixEdge<BigIntUse>(node->child2());
> 
> And here.

ditto.

> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:212
> > +
> 
> nit: remove blank line. I also think the comment above is unnecessary.

Removed the blank line, kept the comment. I generally find jsc to have too few comments, and prefer to leave comments when I see some non-obvious code; but I admit it is a personal preference.

> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:213
> > +            } else {
> 
> nit: this else is not needed.

It is: the if/else if before do not end in a return!

> > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:1181
> > +        case Dec:
> 
> I think rules for prediction propagation are missing there.

Good catch, this is not a phase I understand well, and I had missed it.

> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:666
> > +    auto bytecode = currentInstruction->as<OpToNumber>();
> 
> I think you want to do `OpToNumeric`.

Good catch.

> > Source/JavaScriptCore/jit/JITOpcodes.cpp:671
> > +    // FIXME: fill in!
> 
> I don't understand this FIXME here. Can you create a bug and elaborate on
> that?

oops, this fixme is from before I wrote the function. Removed.

> > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:844
> > +    auto bytecode = currentInstruction->as<OpToNumber>();
> 
> Ditto.

Fixed.

> > Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:851
> > +    SDjkadakdhsakj;
> 
> I suspect this is a compilation error.

Oops, yeah I put this in to remember to look again at this function before sending the code for review, expecting it would fail to compile. Turns out it is in an #if and as a result I forgot it in.
Fixed.

> 
> > Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:464
> > +        result = JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(resultVariant));
> 
> Why do we use here `JSBigInt::inc`, wihle using `JSBigInt::add(...,
> vm.bigIntConstantOne.get())` into `operationInc` and `operationDec`?

Good observation, I did not yet have JSBigInt::{inc, dec} when I wrote operation{Inc, Dec}. I've fixed them to use the JSBigInt::{inc, dec} functions.
This way if I later optimize Inc/Dec it will benefit them.

> > Source/JavaScriptCore/runtime/VM.cpp:396
> > +        bigIntConstantOne.set(*this, JSBigInt::createFrom(*this, 1));
> 
> What happens in case `canUseJIT() == false`? This constant is still being
> used by LLInt on `JSBigInt::inc` or `JSBigInt::dec` and I suspect we will
> have an error in such scenario.

Great catch! I moved it out of the "if (canUseJIT())".

[Robin Morisset]

Created attachment 383164 [details]
Patch

Patch with fixes based on Caio's comments

[Robin Morisset]

Created attachment 383165 [details]
diff

Diff between the current patch and the previous one (in case it makes reviewing easier).

[EWS Watchlist]

Comment on attachment 383164 [details]
Patch

Attachment 383164 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13227941

New failing tests:
stress/arith-negate-on-various-types.js.default
stress/arith-negate-on-various-types.js.no-cjit-collect-continuously
stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling-profiler
stress/arith-negate-on-various-types.js.no-cjit-validate-phases
stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o0
stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate
stress/arith-negate-on-various-types.js.no-ftl
stress/arith-negate-on-various-types.js.no-llint

[Robin Morisset]

(In reply to Build Bot from comment #18)
> Comment on attachment 383164 [details]
> Patch
> 
> Attachment 383164 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/13227941
> 
> New failing tests:
> stress/arith-negate-on-various-types.js.default
> stress/arith-negate-on-various-types.js.no-cjit-collect-continuously
> stress/arith-negate-on-various-types.js.ftl-no-cjit-validate-sampling-
> profiler
> stress/arith-negate-on-various-types.js.no-cjit-validate-phases
> stress/arith-negate-on-various-types.js.ftl-no-cjit-b3o0
> stress/arith-negate-on-various-types.js.ftl-no-cjit-no-inline-validate
> stress/arith-negate-on-various-types.js.no-ftl
> stress/arith-negate-on-various-types.js.no-llint

I found the problem: I was wrongly using unaryArithShouldSpeculateInt32. Turns out that this function (contrary to unaryArithShouldSpeculateIn52) calls the "ForArithmetic" version of shouldSpeculateInt32OrBoolean, meaning that it proceeds by elimination and declares that anything that is not Int52 or Double must be Int32.
I used it in the fixup of ArithNegate/ValueNegate, which can in theory receive any weird thing (such as a string), and this caused an unconditional speculation, leading to a recompilation loop.

I have the (trivial, one-line) fix, but before I can send it back to the EWS bots, I must first fix https://bugs.webkit.org/show_bug.cgi?id=202832 that was rolled-out in the meantime.

[Robin Morisset]

Created attachment 383526 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 383526 [details]
Patch

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

Nice. Early comments.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:499
> +        JSBigInt* result = JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(operandNumeric));
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +        return JSValue::encode(result);

You can do,

RELEASE_AND_RETURN(scope, JSBigInt::inc(globalObject, WTF::get<JSBigInt*>(operandNumeric)));

> Source/JavaScriptCore/dfg/DFGOperations.cpp:521
> +        JSBigInt* result = JSBigInt::dec(globalObject, WTF::get<JSBigInt*>(operandNumeric));
> +        RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +        return JSValue::encode(result);

You can do,

RELEASE_AND_RETURN(scope, JSBigInt::dec(globalObject, WTF::get<JSBigInt*>(operandNumeric)));

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12977
> +    MacroAssembler::Jump notNumber = m_jit.branchIfNotNumber(argumentRegs, scratch);

Why do we go to the slow path when BigInt comes here while Baseline handles BigInt?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:762
>              break;

Don't we need `ToNumeric` implementation in FTL?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:673
> +    Jump isCell = jump();

Is `isBigInt` better?

> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:852
> +    Jump isCell = jump();

Is `isBigInt` better?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:461
> +    Variant<JSBigInt*, double> resultVariant = argument.toNumeric(globalObject);

Insert exception check here.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:475
> +    Variant<JSBigInt*, double> resultVariant = argument.toNumeric(globalObject);

Insert exception check here.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:597
> +    Variant<JSBigInt*, double> resultVariant = argument.toNumeric(globalObject);

Insert exception check here.

[Robin Morisset]

Created attachment 383645 [details]
Patch

[Robin Morisset]

Thanks a lot for the review. I've applied your suggestions.

[Yusuke Suzuki]

Comment on attachment 383645 [details]
Patch

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

> Source/JavaScriptCore/bytecode/BytecodeList.rb:355
> +op_group :ValueProfiledUnaryOp,

I believe, eventually, we should convert it to UnaryArithProfile too. We do not have the case that is getting non numeric values.\
Can you add a FIXME here?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3117
>      switch (pc->opcodeID()) {

When using ValueProfile/ArithProfile, you also need linking code in CodeBlock::finishCreationCommon.
Let's add OpInc, OpDec, OpToNumeric etc.'s handlings.

I recommend grepping all the source with OpNegate / op_negate to find necessary handlings :)
And IIRC we also need the other handlings when using ValueProfile. So I recommend grepping ToNumber / op_to_number to add all necessary handlings. (like, FOR_EACH_OPCODE_WITH_VALUE_PROFILE).

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:936
> +        // FIXME: support some form of constant folding here.

Can you add an URL to bugzilla?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:952
> +            if (BinaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->binaryArithProfileForBytecodeIndex(m_currentIndex)) {
> +                observed = arithProfile->observedResults();
> +            } else if (UnaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->unaryArithProfileForBytecodeIndex(m_currentIndex)) {

Don't need `{` and `}`.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:195
> +            Node* nodeConstantOne;

This should be defined in braces.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12980
> +    addSlowPathGenerator(slowPathCall(notBigInt, this, operationToNumeric, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), argumentRegs));

This slow-path-generator call returns here. So we will call operationToNumeric twice, this is not correct.

We should do,

JumpList slowCases;
slowCases.append(m_jit.branchIfNotBigInt(argumentRegs.payloadGPR()));
...
slowCases.append(m_jit.branchIfNotNumber(argumentRegs, scratch));
...

and just before calling jsValueResult, 

addSlowPathGenerator(slowPathCall(slowCases, this, operationToNumeric, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), argumentRegs));

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6953
> +        }

We need `abstractValue(m_node->child1()).m_type & (SpecBytecodeNumber | SpecBigInt)` => false path, which should also set `setJSValue()`.

[Robin Morisset]

Created attachment 383768 [details]
Patch

[Robin Morisset]

Thanks a lot for the review!

(In reply to Yusuke Suzuki from comment #24)
> Comment on attachment 383645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383645&action=review
> 
> > Source/JavaScriptCore/bytecode/BytecodeList.rb:355
> > +op_group :ValueProfiledUnaryOp,
> 
> I believe, eventually, we should convert it to UnaryArithProfile too. We do
> not have the case that is getting non numeric values.\
> Can you add a FIXME here?

We currently only specialize for Number/BigInt/Everything else, but in theory we could have special paths for string for example. I can add a FIXME if you want, but I am not entirely sure that ValueProfile is wrong here.
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3117
> >      switch (pc->opcodeID()) {
> 
> When using ValueProfile/ArithProfile, you also need linking code in
> CodeBlock::finishCreationCommon.
> Let's add OpInc, OpDec, OpToNumeric etc.'s handlings.
> 
> I recommend grepping all the source with OpNegate / op_negate to find
> necessary handlings :)
> And IIRC we also need the other handlings when using ValueProfile. So I
> recommend grepping ToNumber / op_to_number to add all necessary handlings.
> (like, FOR_EACH_OPCODE_WITH_VALUE_PROFILE).

I thought I had already grepped, but I visibly missed a bunch of places :-(. Thanks for noticing them.

> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:936
> > +        // FIXME: support some form of constant folding here.
> 
> Can you add an URL to bugzilla?

Done.
 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:952
> > +            if (BinaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->binaryArithProfileForBytecodeIndex(m_currentIndex)) {
> > +                observed = arithProfile->observedResults();
> > +            } else if (UnaryArithProfile* arithProfile = m_inlineStackTop->m_profiledBlock->unaryArithProfileForBytecodeIndex(m_currentIndex)) {
> 
> Don't need `{` and `}`.

Fixed

> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:195
> > +            Node* nodeConstantOne;
> 
> This should be defined in braces.

It is already in braces (around the entire Inc/Dec case).

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12980
> > +    addSlowPathGenerator(slowPathCall(notBigInt, this, operationToNumeric, resultRegs, TrustedImmPtr::weakPointer(m_graph, m_graph.globalObjectFor(node->origin.semantic)), argumentRegs));
> 
> This slow-path-generator call returns here. So we will call
> operationToNumeric twice, this is not correct.
> 
> We should do,
> 
> JumpList slowCases;
> slowCases.append(m_jit.branchIfNotBigInt(argumentRegs.payloadGPR()));
> ...
> slowCases.append(m_jit.branchIfNotNumber(argumentRegs, scratch));
> ...
> 
> and just before calling jsValueResult, 
> 
> addSlowPathGenerator(slowPathCall(slowCases, this, operationToNumeric,
> resultRegs, TrustedImmPtr::weakPointer(m_graph,
> m_graph.globalObjectFor(node->origin.semantic)), argumentRegs));

Thank you for the explanation, I was super confused by this code and what addSlowPathGenerator/slowPathCall was doing. I did not realize it could return (I was seeing it as some kind of OSR exit). Fixed.

> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6953
> > +        }
> 
> We need `abstractValue(m_node->child1()).m_type & (SpecBytecodeNumber |
> SpecBigInt)` => false path, which should also set `setJSValue()`.

Oops, I had planned for it and forgot. Fixed.

[Robin Morisset]

Created attachment 383813 [details]
Patch

simple rebase

[Robin Morisset]

Created attachment 383818 [details]
Patch

rebased, this time correctly.

[Yusuke Suzuki]

Comment on attachment 383818 [details]
Patch

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

Looks good overall. I found several issues in DFG, but other place seems good!

> Source/JavaScriptCore/bytecode/ArithProfile.h:84
> +    ObservedResults(uint8_t bits)

Put `explicit`.

> Source/JavaScriptCore/bytecode/ArithProfile.h:98
> +    uint8_t m_bits;

I recommend using `uint8_t m_bits { 0 }`. And define default constructor like, `ObservedResults() = default;`.

> Source/JavaScriptCore/bytecode/ArithProfile.h:177
>   */

Let's define default constructor like, `ArithProfile() = default`.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12966
> +    DFG_ASSERT(m_jit.graph(), node, node->child1().useKind() == UntypedUse, node->child1().useKind());
> +    JSValueOperand argument(this, node->child1());
> +    JSValueRegsTemporary result(this, Reuse, argument);
> +
> +    JSValueRegs argumentRegs = argument.jsValueRegs();
> +    JSValueRegs resultRegs = result.regs();
> +
> +    argument.use();
> +
> +    GPRTemporary temp(this);
> +    GPRReg scratch = temp.gpr();

I think this is not correct. After calling `use()`, we allocate a new GPRTemporary. And we are using `Reuse` for JSValueRegsTemporary.
This means that argumentRegs, resultRegs, and scratch can be the same.
I recommend using simple way first, not using manual `use()` call, Reuse tag, and UseChildrenCalledExplicitly.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12974
> +    slowCases.append(m_jit.branchIfNotBigInt(argumentRegs.payloadGPR()));
> +        
> +    notCell.link(&m_jit);
> +    slowCases.append(m_jit.branchIfNotNumber(argumentRegs, scratch));

If `argumentRegs.payloadGPR()` is JSBigInt, it falls though here, and jumps with `m_jit.branchIfNotNumber`. So, JSBigInt is always taking a slow path.

[Caio Lima]

Comment on attachment 383818 [details]
Patch

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

Overall LGTM as well. I left some comments about prediction propagation and fixup phase.

> Source/JavaScriptCore/dfg/DFGBackwardsPropagationPhase.cpp:405
> +        case ToNumeric: {

I think we could also benefit from of backwards propagation for `Inc` and `Dec`. Maybe we can clear out following flags: `NodeBytecodeNeedsNegZero` or `NodeBytecodeUsesAsOther`.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:212
> +            } else {

It seems that we don't want to have a Int52 specialization here. Did you check if it is perf neutral? If yes, do you mind share the numbers in the bug?

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:306
> +                        changed |= mergePrediction(SpecInt52Any);

I think that having `SpecInt52Any` propagated while not fixing up this use during fixup phase can cause some performance issues. Imagine we have the following DFG:

```
...
@10: Inc(@9:DoubleRepUse,...) // Fixup will change this to double rep use
@11: ArithAdd(@10:Int52, @8:Int52) // This will cause an OSR due to BadType
...
```

[Robin Morisset]

Created attachment 383913 [details]
Patch

Fixing the problems pointed by Yusuke and Caio. Thanks for the reviews.

[Robin Morisset]

Created attachment 383914 [details]
diff from previous patch

[Yusuke Suzuki]

Comment on attachment 383913 [details]
Patch

r=me

[Robin Morisset]

Comment on attachment 383913 [details]
Patch

This version of the patch visibly broke some of the tests.. investigating.

[Robin Morisset]

On problem is trivial: setNonCellTypeForNode instead of setTypeForNode in the default case for ToNumeric.

But there is another problem, which causes segfaults, only when the FTL is on, that I am still investigating.

[Robin Morisset]

Created attachment 383934 [details]
Patch

This patch fixes three problems:
- LowLevelAssembler64.asm used UnaryArithProfile::m_bits where it should have used the BinaryArithProfile::m_bits version. This is harmless currently (and unrelated to this patch) but would have crashed if we ever make these two have different sizes)
- I had forgotten to check for isCell in FTLLowerDFGToB3::compileToNumeric, before testing for IsBigInt, causing a crash whenever doing undefined++ or null++
- I was using setNonCellTypeForNode instead of setTypeForNode for the default case in DFGAbstractInterpreterInlines.h

[WebKit Commit Bot]

Comment on attachment 383934 [details]
Patch

Clearing flags on attachment: 383934

Committed r252680: <https://trac.webkit.org/changeset/252680>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57346871>