Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: fully define Math.sqrt #3345

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Normative: fully define Math.sqrt #3345

wants to merge 2 commits into from

Conversation

michaelficarra
Copy link
Member

This matches the behaviour that web engines today already display with the WebAssembly f64.sqrt instruction.

/cc @sunfishcode

@michaelficarra michaelficarra added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. needs data This PR needs more information; such as web compatibility data, “web reality” (what all engines do)… needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Jun 6, 2024
@anba
Copy link
Contributor

anba commented Jun 6, 2024

Implementations most likely call into std::sqrt, which computes the exact result. (std::sqrt is required to compute the exact result to be IEEE-754 compliant.)

@sunfishcode
Copy link
Member

In addition to changing the semantics for Math.sqrt, should this PR also remove the mention of sqrt from this list of functions where the behavior is "not precisely specified here"?

JIT-based implementations will likely use the sqrt instructions available on all popular general-purpose CPU architectures, which are correctly rounded.

And in case any implementations using the fdlibm sqrt implementation, which is referenced in ECMA-262 here, that implementation also claims to be correctly rounded in its comments.

@michaelficarra
Copy link
Member Author

I did some tests on this today. I ran 10 million random non-negative doubles through SM, JSC, V8, XS, LibJS, and Chakra. Chakra had a lot of divergence, but the other engines all agreed on all inputs. It's not conclusive, but it seems pretty likely that the large cohort are all currently implemented in a way that will always produce the exact result.

@phoddie
Copy link

phoddie commented Jun 27, 2024

Thank you for running the test to characterize the current level of conformance to the proposed change and for including XS in that.

XS has the option to use fdlibm for math functions known to diverge, but that does not include sqrt. That means the test you ran used the host implementation of sqrt. It would be interesting to repeat your test on an embedded device or two. Those runtimes are not always as precise so there's a greater chance of a difference. Is it possible for me to run your test?

@michaelficarra
Copy link
Member Author

Okay, I think I found the reason for Chakra's divergence, and it was due to differences in how they print floats, not how they do Math.sqrt. I've updated my test to not rely on representing floats as text and now all engines agree on all 10M inputs. Here's my updated test (run through eshost -s):

function simpleHashInt32(n, accum = 0) {
  accum = 0 | (accum + n);
  accum = 0 | (accum + (accum << 10));
  accum ^= accum >> 6;
  accum = 0 | (accum + (accum << 3));
  accum ^= accum >> 11;
  accum = 0 | (accum + (accum << 15));
  return accum;
}

let taInt32 = new Int32Array(2);
let taFloat64 = new Float64Array(taInt32.buffer);
function intsToFloat(low, high) {
    taInt32[0] = low;
    taInt32[1] = high;
    return taFloat64[0];
}
function floatToInts(f) {
  taFloat64[0] = f;
  return [taInt32[0], taInt32[1]];
}

let runningInputHash = 0;
let runningSqrtHash = 0;
let state = 1;
let tests = 0;
for (let tests = 0; tests < 10000000;) {
  let low = simpleHashInt32(state);
  let high = simpleHashInt32(low);
  state = high;
  let candidate = intsToFloat(low, high);
  if (!Number.isFinite(candidate) || candidate <= 0) {
    continue;
  }
  ++tests;
  runningInputHash = simpleHashInt32(low, simpleHashInt32(high, runningInputHash))
  let [sqrtLow, sqrtHigh] = floatToInts(Math.sqrt(candidate));
  runningSqrtHash = simpleHashInt32(sqrtLow, simpleHashInt32(sqrtHigh, runningSqrtHash))
}

print(runningInputHash);
print(runningSqrtHash);

which outputs

670366763
-1672354165

on all of the above engines for me.

@phoddie
Copy link

phoddie commented Jun 28, 2024

@michaelficarra – thank you for posting the test script. I ran an abbreviated version (1M iterations) on ESP32-S3. The results match XS on macOS.

@michaelficarra
Copy link
Member Author

@sunfishcode Would you like to attend the presentation of this to TC39 as an invited expert and possibly co-champion?

@sunfishcode
Copy link
Member

@michaelficarra Yes, I would like to attend, as an invited expert.

@michaelficarra
Copy link
Member Author

@sunfishcode Okay, I've initiated the process at https://github.com/tc39/Admin-and-Business/issues/459. The chairs may reach out for additional on-boarding information soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. needs data This PR needs more information; such as web compatibility data, “web reality” (what all engines do)… needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants