Bug 141546 – ArithSqrt should not be conditional on supportsFloatingPointSqrt

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

ArithSqrt should not be conditional on supportsFloatingPointSqrt

[Benjamin Poulain]

Created attachment 246495 [details]
Patch

[Geoffrey Garen]

Comment on attachment 246495 [details]
Patch

r=me

[Filip Pizlo]

Comment on attachment 246495 [details]
Patch

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

You should fix the register allocation bug.  Also, since your tests currently don't disable concurrent JIT, they will be flaky.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3549
> +    FPRTemporary result(this, op1);

This should be FPRResult instead of FPRTemporary when we're doing flushRegisters().  Otherwise weird and uncommon register allocation bugs will happen.

> Source/JavaScriptCore/tests/stress/math-sqrt-basics-disable-architecture-specific-optimizations.js:1
> +//@ run("no-architecture-specific-optimizations", "--enableArchitectureSpecificOptimizations=false")

Should probably be:

run("no-architecture-specific-optimizations", "--enableArchitectureSpecificOptimizations=false", *NO_CJIT_OPTIONS)

For good measure, you should probably also add a second line that runs in FTL as well.

> Source/JavaScriptCore/tests/stress/math-sqrt-basics-disable-architecture-specific-optimizations.js:10
> +    for (var i = 0; i < 1000000; ++i) {

Only 10000 runs are needed with cjit turned off.

> Source/JavaScriptCore/tests/stress/math-sqrt-basics.js:8
> +    for (var i = 0; i < 1000000; ++i) {

Only 10000 runs are needed.

[Benjamin Poulain]

Created attachment 246552 [details]
Patch

[Geoffrey Garen]

Comment on attachment 246552 [details]
Patch

r=me

[Benjamin Poulain]

Committed r180085: <http://trac.webkit.org/changeset/180085>