Never hchange nan-hole to hole or hole to nan-hole.

Never hchange nan-hole to hole or hole to nan-hole.
Only allow changing hole to nan if all uses allow undefined as nan.

R=danno@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16183

[Toon Verwaest, 2013-08-05 09:29:58]

PTAL

[Toon Verwaest, 2013-08-07 14:16:02]

Entirely changed the approach. PTAL again; only ia32 for now.

[danno, 2013-08-13 12:55:03]

https://codereview.chromium.org/22152003/diff/17024/src/hydrogen-mark-deoptim...
File src/hydrogen-mark-deoptimize.cc (right):

https://codereview.chromium.org/22152003/diff/17024/src/hydrogen-mark-deoptim...
src/hydrogen-mark-deoptimize.cc:74: if
(HChange::cast(current)->convert_undefined_to_nan()) {
shouldn't this method be called "can_context_undefined_to_nan"?

https://codereview.chromium.org/22152003/diff/17024/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/22152003/diff/17024/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:2480: if
(instr->hydrogen()->representation().IsTagged()) {
What about smi representations?

[Toon Verwaest, 2013-08-13 13:18:55]

Addressed comment

https://chromiumcodereview.appspot.com/22152003/diff/17024/src/hydrogen-mark-...
File src/hydrogen-mark-deoptimize.cc (right):

https://chromiumcodereview.appspot.com/22152003/diff/17024/src/hydrogen-mark-...
src/hydrogen-mark-deoptimize.cc:74: if
(HChange::cast(current)->convert_undefined_to_nan()) {
On 2013/08/13 12:55:03, danno wrote:
> shouldn't this method be called "can_context_undefined_to_nan"?

Done.

https://chromiumcodereview.appspot.com/22152003/diff/17024/src/ia32/lithium-c...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/22152003/diff/17024/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:2480: if
(instr->hydrogen()->representation().IsTagged()) {
The hole never escapes as smi; only as double (nan) and tagged.

On 2013/08/13 12:55:03, danno wrote:
> What about smi representations?

[danno, 2013-08-13 13:20:29]

lgtm

[Toon Verwaest, 2013-08-13 13:55:50]

Ported to ARM / X64, PTAL

[Rodolph Perfetta, 2013-08-13 15:08:14]

dbc

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2161: void LCodeGen::EmitFalseBranch(InstrType
instr, Condition cc) {
cc is actually a valid ARM condition code (carry clear) it would be better to
use another name.

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2426: __ mov(ip, Operand(Handle<Object>(cell)));
Shouldn't this be: mov(ip, factory()->the_hole_value());

if you allocate a new cell then the comparison will always fails no?

[Toon Verwaest, 2013-08-13 15:16:50]

Hi Rodolph,

thanks for the DBC!
I have an inline question.

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2161: void LCodeGen::EmitFalseBranch(InstrType
instr, Condition cc) {
I used the same signature as the function above (EmitBranch). Do you suggest to
change that as well?

On 2013/08/13 15:08:14, Rodolph Perfetta wrote:
> cc is actually a valid ARM condition code (carry clear) it would be better to
> use another name.

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2426: __ mov(ip, Operand(Handle<Object>(cell)));
Makes a lot of sense. Wrongly copy pasted from below.

On 2013/08/13 15:08:14, Rodolph Perfetta wrote:
> Shouldn't this be: mov(ip, factory()->the_hole_value());
> 
> if you allocate a new cell then the comparison will always fails no?
>

[Rodolph Perfetta, 2013-08-13 15:22:02]

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/22152003/diff/35001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2161: void LCodeGen::EmitFalseBranch(InstrType
instr, Condition cc) {
It would be better to change it above too. If this code get copy/pasted and the
carry clear condition is then used, it will make for a very tricky bug to find.

On 2013/08/13 15:16:50, Toon Verwaest wrote:
> I used the same signature as the function above (EmitBranch). Do you suggest
to
> change that as well?
> 
> On 2013/08/13 15:08:14, Rodolph Perfetta wrote:
> > cc is actually a valid ARM condition code (carry clear) it would be better
to
> > use another name.
>

[Toon Verwaest, 2013-08-13 17:12:04]

Ok, done.

[Toon Verwaest, 2013-08-14 08:54:51]

Committed patchset #7 manually as r16183.