Collect type feedback on result of Math.[round|ceil|floor]

Collect type feedback on result of Math.[round|ceil|floor]

By recording invocations of these builtins that can return -0, we now learn to not emit Crankshaft code that only handles integer results, avoiding deopt loops.

Committed: https://crrev.com/f36ecaf3a4d61568ca50a20718acce7dd5da9a5f
Cr-Commit-Position: refs/heads/master@{#28215}

[mvstanton, 2015-04-28 14:21:20]

mvstanton@chromium.org changed reviewers:
+ mvstanton@chromium.org

[mvstanton, 2015-04-28 14:21:20]

Looks good, I just have a few consolidation/refactoring comments.

https://codereview.chromium.org/1053143005/diff/90001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1053143005/diff/90001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2377: __ mov(eax, FieldOperand(edi,
JSFunction::kSharedFunctionInfoOffset));
Since this sequence happens three times, maybe it's useful to have a helper
(either in macro assembler or CallIC base class) like

void EmitMissIfNotBuiltin(int builtinId, Register function, Register temp,
Label* miss);

https://codereview.chromium.org/1053143005/diff/90001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2537: __ cmp(eax,
Immediate(Smi::FromInt(kMathRound)));
Maybe it's okay to go to the miss handler if it's any builtin? Weight against
the cost of checking for 3 numbers it might be worth it to just say to heck with
it, if FunctionData is a non-zero Smi, just miss.

https://codereview.chromium.org/1053143005/diff/90001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1053143005/diff/90001/src/ic/ic.cc#newcode2246
src/ic/ic.cc:2246: if (AddressIsOptimizedCode()) {
Could this AddressIsOptimizedCode() { STUB1 } else { STUB2 } sequence be
templatized as a method on CallIC?

https://codereview.chromium.org/1053143005/diff/90001/src/type-info.h
File src/type-info.h (right):

https://codereview.chromium.org/1053143005/diff/90001/src/type-info.h#newcode31
src/type-info.h:31: bool CallIsRoundWithMinusZeroResult(FeedbackVectorICSlot
slot);
Remove Round from the name since it's more general now..

[danno, 2015-04-30 13:34:30]

danno@chromium.org changed reviewers:
+ bmeurer@chromium.org

[danno, 2015-04-30 13:34:32]

First round of comments addresses, PTAL

https://codereview.chromium.org/1053143005/diff/90001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1053143005/diff/90001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2377: __ mov(eax, FieldOperand(edi,
JSFunction::kSharedFunctionInfoOffset));
On 2015/04/28 14:21:20, mvstanton wrote:
> Since this sequence happens three times, maybe it's useful to have a helper
> (either in macro assembler or CallIC base class) like
> 
> void EmitMissIfNotBuiltin(int builtinId, Register function, Register temp,
> Label* miss);

Done.

https://codereview.chromium.org/1053143005/diff/90001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2537: __ cmp(eax,
Immediate(Smi::FromInt(kMathRound)));
On 2015/04/28 14:21:20, mvstanton wrote:
> Maybe it's okay to go to the miss handler if it's any builtin? Weight against
> the cost of checking for 3 numbers it might be worth it to just say to heck
with
> it, if FunctionData is a non-zero Smi, just miss.

Done.

https://codereview.chromium.org/1053143005/diff/90001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/1053143005/diff/90001/src/ic/ic.cc#newcode2246
src/ic/ic.cc:2246: if (AddressIsOptimizedCode()) {
On 2015/04/28 14:21:20, mvstanton wrote:
> Could this AddressIsOptimizedCode() { STUB1 } else { STUB2 } sequence be
> templatized as a method on CallIC?

I'd like to avoid the template in this case, since it seems like a little bit
too much meta template zealotry for just a few lines of code that are pretty
easy to understand, but I probably could be convinced otherwise.

https://codereview.chromium.org/1053143005/diff/90001/src/type-info.h
File src/type-info.h (right):

https://codereview.chromium.org/1053143005/diff/90001/src/type-info.h#newcode31
src/type-info.h:31: bool CallIsRoundWithMinusZeroResult(FeedbackVectorICSlot
slot);
On 2015/04/28 14:21:20, mvstanton wrote:
> Remove Round from the name since it's more general now..

Done.

[mvstanton, 2015-05-04 09:41:22]

A few nits, but beyond that LGTM.

https://codereview.chromium.org/1053143005/diff/190001/src/arm64/code-stubs-a...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:3388: #if 0
Why is this commented out, I guess it should not be...

https://codereview.chromium.org/1053143005/diff/190001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/hydrogen.cc#newcod...
src/hydrogen.cc:8497: NoObservableSideEffectsScope s(this);
Why does this have to be in a NoObservableSideEffectsScope?

https://codereview.chromium.org/1053143005/diff/190001/src/ia32/code-stubs-ia...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/ia32/code-stubs-ia...
src/ia32/code-stubs-ia32.cc:2531: __ cmp(eax,
Immediate(Smi::FromInt(kMathRound)));
Make this look like the other platforms, where you just compare against 0
instead of each interesting builtin id..

https://codereview.chromium.org/1053143005/diff/190001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/type-feedback-vect...
src/type-feedback-vector.cc:318:
Smi::FromInt(CallICStub::kHasReturnedMinusZeroSentinel));
I like this tight control of expectations, thx!

https://codereview.chromium.org/1053143005/diff/190001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/type-info.cc#newco...
src/type-info.cc:150: SmartArrayPointer<char> n =
maybe_round->shared()->DebugName()->ToCString();
You don't need n here.

[danno, 2015-05-04 14:31:56]

Feedback addressed. Please take another look.

https://codereview.chromium.org/1053143005/diff/190001/src/arm64/code-stubs-a...
File src/arm64/code-stubs-arm64.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/arm64/code-stubs-a...
src/arm64/code-stubs-arm64.cc:3388: #if 0
On 2015/05/04 09:41:21, mvstanton wrote:
> Why is this commented out, I guess it should not be...

Done.

https://codereview.chromium.org/1053143005/diff/190001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/hydrogen.cc#newcod...
src/hydrogen.cc:8497: NoObservableSideEffectsScope s(this);
On 2015/05/04 09:41:22, mvstanton wrote:
> Why does this have to be in a NoObservableSideEffectsScope?

Otherwise there are missing simulates after the following instructions. The
simulate only needs to happen after the final HMul.

https://codereview.chromium.org/1053143005/diff/190001/src/ia32/code-stubs-ia...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/ia32/code-stubs-ia...
src/ia32/code-stubs-ia32.cc:2531: __ cmp(eax,
Immediate(Smi::FromInt(kMathRound)));
On 2015/05/04 09:41:22, mvstanton wrote:
> Make this look like the other platforms, where you just compare against 0
> instead of each interesting builtin id..

Done.

https://codereview.chromium.org/1053143005/diff/190001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/type-feedback-vect...
src/type-feedback-vector.cc:318:
Smi::FromInt(CallICStub::kHasReturnedMinusZeroSentinel));
On 2015/05/04 09:41:22, mvstanton wrote:
> I like this tight control of expectations, thx!

Acknowledged.

https://codereview.chromium.org/1053143005/diff/190001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/1053143005/diff/190001/src/type-info.cc#newco...
src/type-info.cc:150: SmartArrayPointer<char> n =
maybe_round->shared()->DebugName()->ToCString();
On 2015/05/04 09:41:22, mvstanton wrote:
> You don't need n here.

Done.

[balazs.kilvady, 2015-05-04 18:51:34]

Please find the MIPS port at https://codereview.chromium.org/1127573004/

[danno, 2015-05-05 07:16:35]

The CQ bit was checked by danno@chromium.org

[danno, 2015-05-05 07:16:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org
Link to the patchset: https://codereview.chromium.org/1053143005/#ps230001
(title: "MIPS port")

[commit-bot: I haz the power, 2015-05-05 07:17:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1053143005/230001

[commit-bot: I haz the power, 2015-05-05 07:55:59]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-05-05 07:56:17]

Patchset 13 (id:??) landed as
https://crrev.com/f36ecaf3a4d61568ca50a20718acce7dd5da9a5f
Cr-Commit-Position: refs/heads/master@{#28215}

[danno, 2015-05-05 16:41:10]

A revert of this CL (patchset #13 id:230001) has been created in
https://codereview.chromium.org/1115973005/ by danno@chromium.org.

The reason for reverting is: All sorts of performance regressions.