[Interpreter] Avoid accessing on-heap literal in VisitLiteral.

[Interpreter] Avoid accessing on-heap literal in VisitLiteral.

Move VisitLiteral to decide what type of literal is being emitted by
checking the raw ASTValue type, instead of the internalized on-heap
value. This is required for concurrent bytecode generation.

As part of this change, the NUMBER AstValue constructor is modified to
try to convert numbers without a dot to SMIs where possible. This is to
maintain the behavior in NewNumber where such numbers are internalized as
SMIs, and ensures that we still emit LdaSmi bytecodes for these values
in the generated bytecode.

BUG=v8:5203
Committed: https://crrev.com/6b5949a8a04a2bc784bc6936cc713b466bfe2044
Cr-Commit-Position: refs/heads/master@{#37931}

[rmcilroy, 2016-07-14 17:03:40]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-14 17:03:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 17:37:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 17:37:34]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-07-14 17:55:45]

rmcilroy@chromium.org changed reviewers:
+ vogelheim@chromium.org

[rmcilroy, 2016-07-14 17:55:46]

Ptal, thanks.

[rmcilroy, 2016-07-18 11:18:46]

rmcilroy@chromium.org changed reviewers:
+ adamk@chromium.org

[rmcilroy, 2016-07-18 11:18:47]

+adamk for parser owners review. PTAL, thanks.

[vogelheim, 2016-07-18 13:01:04]

lgtm

https://codereview.chromium.org/2152853002/diff/1/test/cctest/interpreter/byt...
File test/cctest/interpreter/bytecode_expectations/UnaryOperators.golden
(right):

https://codereview.chromium.org/2152853002/diff/1/test/cctest/interpreter/byt...
test/cctest/interpreter/bytecode_expectations/UnaryOperators.golden:153: /*   46
S> */ B(LdaConstant), U8(0),
[Feel free to ignore; it might be just me who isn't getting this.]

I don't get this... I would have expected your change to strictly increase the
number of SMIs; but this looks like the bytecode generator used to get a
LoadLiteral(Smi*), while now it gets a LoadLiteral(Handle<.>...). Why?

[rmcilroy, 2016-07-18 13:12:19]

https://codereview.chromium.org/2152853002/diff/1/test/cctest/interpreter/byt...
File test/cctest/interpreter/bytecode_expectations/UnaryOperators.golden
(right):

https://codereview.chromium.org/2152853002/diff/1/test/cctest/interpreter/byt...
test/cctest/interpreter/bytecode_expectations/UnaryOperators.golden:153: /*   46
S> */ B(LdaConstant), U8(0),
On 2016/07/18 13:01:04, vogelheim wrote:
> [Feel free to ignore; it might be just me who isn't getting this.]
> 
> I don't get this... I would have expected your change to strictly increase the
> number of SMIs; but this looks like the bytecode generator used to get a
> LoadLiteral(Smi*), while now it gets a LoadLiteral(Handle<.>...). Why? 

Unfortunately not. Previously we would get SMIs for any smi value because
NewNumber would SMIify any NUMBER which was in the smi range (and we used the
actual type of the object created, not the parser representation).

We don't SMI this because the parser specifically transforms +x into x * 1.0 (so
NUMBER_WITH_DOT). I don't convert these values to SMI because there are other
parts of the parser which checks whether the constant is 1 with a dot and does
various other operations on it if so. I suppose I could create a SMI_WITH_DOT
representation for these cases, but it seems a bit overkill, WDYT?

[vogelheim, 2016-07-18 16:18:15]

On 2016/07/18 13:12:19, rmcilroy wrote:
>
https://codereview.chromium.org/2152853002/diff/1/test/cctest/interpreter/byt...
> File test/cctest/interpreter/bytecode_expectations/UnaryOperators.golden
> (right):
> 
>
https://codereview.chromium.org/2152853002/diff/1/test/cctest/interpreter/byt...
> test/cctest/interpreter/bytecode_expectations/UnaryOperators.golden:153: /*  
46
> S> */ B(LdaConstant), U8(0),
> On 2016/07/18 13:01:04, vogelheim wrote:
> > [Feel free to ignore; it might be just me who isn't getting this.]
> > 
> > I don't get this... I would have expected your change to strictly increase
the
> > number of SMIs; but this looks like the bytecode generator used to get a
> > LoadLiteral(Smi*), while now it gets a LoadLiteral(Handle<.>...). Why? 
> 
> Unfortunately not. Previously we would get SMIs for any smi value because
> NewNumber would SMIify any NUMBER which was in the smi range (and we used the
> actual type of the object created, not the parser representation).
> 
> We don't SMI this because the parser specifically transforms +x into x * 1.0
(so
> NUMBER_WITH_DOT). I don't convert these values to SMI because there are other
> parts of the parser which checks whether the constant is 1 with a dot and does
> various other operations on it if so. I suppose I could create a SMI_WITH_DOT
> representation for these cases, but it seems a bit overkill, WDYT?

Yeah, SMI_WITH_DOT does seem a bit much.

I guess what it comes down to is the current code makes the decision too early:
It assumes a value has exactly one type, that it is one thing or another, but
not both. One could instead make it more of a conversion thing, like, IsSMIable,
which would be true for any Number (including those w/ dot) that can be
converted to a Smi. That way, the code that uses it makes the decision on how
'aggressive' the conversion is.

I guess it's not really worth the effort, though...

[adamk, 2016-07-18 17:27:41]

I'm surprised not to see any more test changes; is there any way to get more
coverage for this logic? Or is it already well-covered and it's only the UnaryOp
case that changes?

https://codereview.chromium.org/2152853002/diff/1/src/ast/ast-value-factory.h
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2152853002/diff/1/src/ast/ast-value-factory.h...
src/ast/ast-value-factory.h:168: UNREACHABLE();
Not sure why these AsWhatevers are phrased like this. Simpler code would be:

CHECK_EQ(SMI, type_);
return Smi::FromInt(smi_);

You're just following what else is in this file, but it seemed odd to me.

https://codereview.chromium.org/2152853002/diff/1/src/ast/ast-value-factory.h...
src/ast/ast-value-factory.h:223: if (!IsMinusZero(n) && int_value == n &&
Smi::IsValid(int_value)) {
I'm kind of sad to see this code reproduced again, and slightly differently. I
know we have IsSmiDouble(), which sadly does not return the Smi-valid int.
Consider factoring this out?

[rmcilroy, 2016-07-19 14:31:55]

Addressed review comments, PTAL.

> I'm surprised not to see any more test changes; is there any way to get more
> coverage for this logic? Or is it already well-covered and it's only the
UnaryOp
> case that changes?

The only time it will make a difference is for integer literals that contain a
dot (e.g., 2.0). I guess this is pretty uncommon in real world code, so the only
time we saw it is where the parser explicitly does it (for the +x desuggering to
x * 1.0). I've added an explicit test in PrimitiveReturnStatements for this, as
well as the +x case.


> I guess what it comes down to is the current code makes the decision too
early:
> It assumes a value has exactly one type, that it is one thing or another, but
> not both. One could instead make it more of a conversion thing, like,
IsSMIable,
> which would be true for any Number (including those w/ dot) that can be
> converted to a Smi. That way, the code that uses it makes the decision on how
> 'aggressive' the conversion is.

Yeah, I think the real problem is that the parser tracks HasDot for no real
reason AFAIKT except to identify the +x desugering. Maybe there is something
else I'm missing. If we could get rid of this then there would be no need for
NUMBER_WITH_DOT and we could just always do the SMI conversion early where
possible.


> I guess it's not really worth the effort, though...

Agreed, I think this is good enough for now, once the Parser only feeds Ignition
we can refactor some of this stuff.

https://codereview.chromium.org/2152853002/diff/1/src/ast/ast-value-factory.h
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2152853002/diff/1/src/ast/ast-value-factory.h...
src/ast/ast-value-factory.h:168: UNREACHABLE();
On 2016/07/18 17:27:41, adamk wrote:
> Not sure why these AsWhatevers are phrased like this. Simpler code would be:
> 
> CHECK_EQ(SMI, type_);
> return Smi::FromInt(smi_);
> 
> You're just following what else is in this file, but it seemed odd to me.

Done.

https://codereview.chromium.org/2152853002/diff/1/src/ast/ast-value-factory.h...
src/ast/ast-value-factory.h:223: if (!IsMinusZero(n) && int_value == n &&
Smi::IsValid(int_value)) {
On 2016/07/18 17:27:41, adamk wrote:
> I'm kind of sad to see this code reproduced again, and slightly differently. I
> know we have IsSmiDouble(), which sadly does not return the Smi-valid int.
> Consider factoring this out?

Done.

[rmcilroy, 2016-07-19 14:32:48]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 14:32:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 15:03:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 15:03:25]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-07-19 19:26:31]

lgtm % CHECK issue (feel free to revert to the old code if you'd prefer)

https://codereview.chromium.org/2152853002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2152853002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:149: DCHECK_EQ(STRING, type_);
Oh, sorry, the equivalent code is actually a CHECK (not a DCHECK) since
UNREACHABLE is a fatal crash even in release mode.

[rmcilroy, 2016-07-20 21:34:41]

https://codereview.chromium.org/2152853002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2152853002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:149: DCHECK_EQ(STRING, type_);
On 2016/07/19 19:26:31, adamk wrote:
> Oh, sorry, the equivalent code is actually a CHECK (not a DCHECK) since
> UNREACHABLE is a fatal crash even in release mode.

Good point, done.

[rmcilroy, 2016-07-20 21:34:48]

The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 21:34:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 22:05:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 22:05:20]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-07-21 09:17:23]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-07-21 09:17:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
vogelheim@chromium.org, adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2152853002/#ps40001
(title: "DCHECK to CHECK")

[commit-bot: I haz the power, 2016-07-21 09:17:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 09:18:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-21 09:20:28]

Description was changed from

==========
[Interpreter] Avoid accessing on-heap literal in VisitLiteral.

Move VisitLiteral to decide what type of literal is being emitted by
checking the raw ASTValue type, instead of the internalized on-heap
value. This is required for concurrent bytecode generation.

As part of this change, the NUMBER AstValue constructor is modified to
try to convert numbers without a dot to SMIs where possible. This is to
maintain the behavior in NewNumber where such numbers are internalized as
SMIs, and ensures that we still emit LdaSmi bytecodes for these values
in the generated bytecode.

BUG=v8:5203
==========

to

==========
[Interpreter] Avoid accessing on-heap literal in VisitLiteral.

Move VisitLiteral to decide what type of literal is being emitted by
checking the raw ASTValue type, instead of the internalized on-heap
value. This is required for concurrent bytecode generation.

As part of this change, the NUMBER AstValue constructor is modified to
try to convert numbers without a dot to SMIs where possible. This is to
maintain the behavior in NewNumber where such numbers are internalized as
SMIs, and ensures that we still emit LdaSmi bytecodes for these values
in the generated bytecode.

BUG=v8:5203

Committed: https://crrev.com/6b5949a8a04a2bc784bc6936cc713b466bfe2044
Cr-Commit-Position: refs/heads/master@{#37931}
==========

[commit-bot: I haz the power, 2016-07-21 09:20:30]

Patchset 3 (id:??) landed as
https://crrev.com/6b5949a8a04a2bc784bc6936cc713b466bfe2044
Cr-Commit-Position: refs/heads/master@{#37931}