[webkit-reviews] review denied: [Bug 124780] Kraken causes a lot of slow path calls when using Math.pow() : [Attachment 220051] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Romain Perier
<romain.perier at gmail.com>'s request for review:
Bug 124780: Kraken causes a lot of slow path calls when using Math.pow()
https://bugs.webkit.org/show_bug.cgi?id=124780

Attachment 220051: Patch
https://bugs.webkit.org/attachment.cgi?id=220051&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=220051&action=review


> Source/JavaScriptCore/jit/ThunkGenerators.cpp:915
> +    jit.neg32(SpecializedThunkJIT::regT0);
> +    jit.divDouble(SpecializedThunkJIT::fpRegT0,
SpecializedThunkJIT::fpRegT1);
> +    jit.moveDouble(SpecializedThunkJIT::fpRegT1,
SpecializedThunkJIT::fpRegT0);
> +    jit.loadDouble(&oneConstant, SpecializedThunkJIT::fpRegT1);
> +    exponentIsPositive.link(&jit);

I have the feeling jit.neg32(SpecializedThunkJIT::regT0); does not work for
INT_MIN (since -INT_MIN > INT_MAX). Am I missing something here?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:947
> +    nonIntExponent.link(&jit);
> +    jit.loadDoubleArgument(1, SpecializedThunkJIT::fpRegT1,
SpecializedThunkJIT::regT0);
> +    jit.loadDouble(&negativeHalfConstant, SpecializedThunkJIT::fpRegT2);
> +    MacroAssembler::Jump exponentIsInverseSqrt =
jit.branchDouble(MacroAssembler::DoubleEqual, SpecializedThunkJIT::fpRegT1,
SpecializedThunkJIT::fpRegT2);
>  
> -	   SpecializedThunkJIT::JumpList doubleResult;
> -	   jit.branchConvertDoubleToInt32(SpecializedThunkJIT::fpRegT1,
SpecializedThunkJIT::regT0, doubleResult, SpecializedThunkJIT::fpRegT0);
> -	   jit.returnInt32(SpecializedThunkJIT::regT0);
> -	   doubleResult.link(&jit);
> -	   jit.returnDouble(SpecializedThunkJIT::fpRegT1);
> -    } else
> -	   jit.appendFailure(nonIntExponent);
> +    jit.callDoubleToDoublePreservingReturn(BinaryDoubleOpWrapper(jsPow));
> +    jit.returnDouble(SpecializedThunkJIT::fpRegT0);
>  
> +    exponentIsInverseSqrt.link(&jit);
> +    jit.loadDouble(&oneConstant, SpecializedThunkJIT::fpRegT1);
> +    jit.sqrtDouble(SpecializedThunkJIT::fpRegT0,
SpecializedThunkJIT::fpRegT0);
> +    jit.divDouble(SpecializedThunkJIT::fpRegT0,
SpecializedThunkJIT::fpRegT1);
> +    jit.returnDouble(SpecializedThunkJIT::fpRegT1);

The branch using jit.sqrtDouble should be behind test for
jit.supportsFloatingPointSqrt().