Skip to content

Conversation

ThomsonTan
Copy link
Collaborator

Interpreter always go to slow path of Math.pow because the perf win is not substantial in interpreter, and but could give more accurate float point result. This leads to result inconsistency between JIT code and interpreter. This fix checks fast path in interpreter to make the result consistent.

@ThomsonTan
Copy link
Collaborator Author

@rajatd could you please review?

@@ -293,6 +294,11 @@ namespace Js
// pow([+/-] 1, Infinity) = NaN according to javascript, but not for CRT pow.
return JavascriptNumber::NaN;
}
else if (TryGetInt32Value(y, &intY))
Copy link
Contributor

@rajatd rajatd Aug 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run the benchmarks just to be sure. #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Just run though Octane, Kraken and SunSpider again last build from master. No regression detected.


In reply to: 73776762 [](ancestors = 73776762)

@rajatd
Copy link
Contributor

rajatd commented Aug 6, 2016

:shipit:

@rajatd
Copy link
Contributor

rajatd commented Aug 6, 2016

@dotnet-bot test Ubuntu ubuntu_linux_release_static

@ThomsonTan
Copy link
Collaborator Author

@dotnet-bot test this please.

@chakrabot chakrabot merged commit bca1d6b into chakra-core:master Aug 8, 2016
chakrabot pushed a commit that referenced this pull request Aug 8, 2016
…to make the result consistent with JIT code

Merge pull request #1396 from ThomsonTan:FixMathPowInconsistency

Interpreter always go to slow path of Math.pow because the perf win is not substantial in interpreter, and but could give more accurate float point result. This leads to result inconsistency between JIT code and interpreter.  This fix checks fast path in interpreter to make the result consistent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants