Don't MISS if you read the hole from certain FastHoley arrays.

Don't MISS if you read the hole from certain FastHoley arrays.

If the array's map is the initial FastHoley array map, and the array prototype
chain is undisturbed and empty of elements, then keyed loads can convert the
load of a hole to undefined.

BUG=

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

[mvstanton, 2015-04-22 14:07:45]

Patchset #2 (id:20001) has been deleted

[mvstanton, 2015-04-22 14:07:53]

Patchset #1 (id:1) has been deleted

[mvstanton, 2015-04-23 11:48:07]

Patchset #3 (id:80001) has been deleted

[mvstanton, 2015-04-23 11:48:14]

Patchset #2 (id:60001) has been deleted

[mvstanton, 2015-04-23 11:50:29]

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

[mvstanton, 2015-04-23 11:50:30]

Hi Jakob,
Here is the change we discussed yesterday. Have a look when you get a chance,
thanks!
--Michael

[abigail.schenk6, 2015-04-23 12:59:28]

abigail.schenk6@gmail.com changed reviewers:
+ abigail.schenk6@gmail.com

[abigail.schenk6, 2015-04-23 12:59:28]

lgtm

[Jakob Kummerow, 2015-04-23 13:40:00]

LGTM with a bunch of comments, mostly nits though. Happy to take another look if
you want.

MIPS folks, can you please take a look at the lithium-codegen-mips* changes?

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-arm6...
File src/arm64/lithium-arm64.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-arm6...
src/arm64/lithium-arm64.cc:1727: return (instr->RequiresHoleCheck() ||
Suggestion: I'd model this after the case below (lines 1740 ff.):

      LInstruction* result =
          DefineAsRegister(new(zone()) LLoadKeyedFixed(elements, key, temp));
      if (instr->RequiresHoleCheck() ||
          (instr->hole_mode == ... && info()->IsStub()) {
        result = AssignEnvironment(result);
      }
      return result;

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-code...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-code...
src/arm64/lithium-codegen-arm64.cc:3626: __ ldr(result, FieldMemOperand(result,
Cell::kValueOffset));
nit: s/ldr/Ldr/ (always use MacroAssembler instructions on arm64).

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-code...
src/arm64/lithium-codegen-arm64.cc:3631: __ bind(&done);
nit: s/bind/Bind/

https://codereview.chromium.org/1100083002/diff/100001/src/code-stubs-hydroge...
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/code-stubs-hydroge...
src/code-stubs-hydrogen.cc:582: if (casted_stub()->elements_kind() ==
FAST_HOLEY_ELEMENTS &&
Why do we need to check for elements kind again? The IC/handler compilers
already do that when computing the convert_hole_to_undefined flag, no?

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen-instructi...
src/hydrogen-instructions.h:6614: kBitsForBaseOffset = 25,
I think instead of moving |dehoisted| out of this bitfield, I'd rather shrink
the base offset field to 24 bits. Is there a reason not to do that?

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen.cc#newcod...
src/hydrogen.cc:6995: Map* holey_double_elements =
Suggestion: "bool holey_double_elements = *map == ..."
That would make subsequent code look a bit cleaner; but I don't feel strongly
about it.

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codeg...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:3195: __ mov(result,
isolate()->factory()->array_protector());
You can collapse this into one instruction:

__ cmp(Operand::ForCell(isolate()->factory()->array_protector()),
       Immediate(Smi::FromInt(1)));

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:3197: Immediate(Smi::FromInt(1)));
Please factor out the magic 0/1 constants as "static const int
kArrayProtectorValid = 1" (in class Isolate) or similar.

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (left):

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32....
src/ia32/lithium-ia32.cc:2232: if ((instr->is_external() ||
instr->is_fixed_typed_array()) ?
Yo dawg, I heard you like conditions, so I put a conditional into your
condition, so you can check a condition while you check a condition...

+1 to cleaning that up! :-)

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32....
src/ia32/lithium-ia32.cc:2232: bool needs_environment;
We used to have to initialize such variables to keep all compilers happy... not
sure if that's still necessary.

https://codereview.chromium.org/1100083002/diff/100001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/ic/ic-compiler.cc#...
src/ic/ic-compiler.cc:115: bool convert_hole_to_undefined =
Checking for a pristine array prototype is pointless here, because this is
compiled so early that it's pristine anyway, and the stub is doing the check,
right? Maybe add a comment about the latter, roughly:
// No need to check for an elements-free prototype chain here, the generated
// stub code needs to check that dynamically anyway.

[paul.l..., 2015-04-23 15:34:22]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-04-23 15:34:23]

lgtm, with one nit.

https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-cod...
File src/mips64/lithium-codegen-mips64.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-cod...
src/mips64/lithium-codegen-mips64.cc:3345: __ lw(scratch,
FieldMemOperand(result, Cell::kValueOffset));
nit: This should probably be 'ld'. I say 'probably' since in general values are
64-bit. But you are using just a bit flag here, so it's fine (as long as we
never go to big-endian mips64, which we have no plans for :) 

It might be worth a comment here that you are using just LS bit(s) of value.

[balazs.kilvady, 2015-04-23 15:34:39]

On 2015/04/23 13:40:00, Jakob wrote:
> LGTM with a bunch of comments, mostly nits though. Happy to take another look
if
> you want.
> 
> MIPS folks, can you please take a look at the lithium-codegen-mips* changes?

In src/mips64/lithium-codegen-mips64.cc, line 3345
__ lw(scratch, FieldMemOperand(result, Cell::kValueOffset)); very likely should
be
__ ld(scratch, FieldMemOperand(result, Cell::kValueOffset));
I will test it.

Also I guess at line #3359 of src/arm/lithium-codegen-arm.cc
__ LoadRoot(scratch, Heap::kArrayProtectorRootIndex); should be
__ LoadRoot(result, Heap::kArrayProtectorRootIndex);
And in both of the matching MIPS files too.

[balazs.kilvady, 2015-04-23 16:24:34]

balazs.kilvady@imgtec.com changed reviewers:
+ balazs.kilvady@imgtec.com

[balazs.kilvady, 2015-04-23 16:24:37]

https://codereview.chromium.org/1100083002/diff/100001/src/arm/lithium-codege...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:3359: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
The result register should be used instead of scratch in LoadRoot like in
src/arm64/lithium-codegen-arm64.cc.

https://codereview.chromium.org/1100083002/diff/100001/src/mips/lithium-codeg...
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/mips/lithium-codeg...
src/mips/lithium-codegen-mips.cc:3295: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
The result register should be used instead of scratch in LoadRoot like in
src/arm64/lithium-codegen-arm64.cc.

https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-cod...
File src/mips64/lithium-codegen-mips64.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-cod...
src/mips64/lithium-codegen-mips64.cc:3344: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
The result register should be used instead of scratch in LoadRoot like in
src/arm64/lithium-codegen-arm64.cc.

[mvstanton, 2015-04-27 07:57:13]

Thanks guys, addressed comments. Much appreciated,
--Michael

https://codereview.chromium.org/1100083002/diff/100001/src/arm/lithium-codege...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:3359: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
On 2015/04/23 16:24:37, balazs.kilvady wrote:
> The result register should be used instead of scratch in LoadRoot like in
> src/arm64/lithium-codegen-arm64.cc.

Done.

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-arm6...
File src/arm64/lithium-arm64.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-arm6...
src/arm64/lithium-arm64.cc:1727: return (instr->RequiresHoleCheck() ||
On 2015/04/23 13:40:00, Jakob wrote:
> Suggestion: I'd model this after the case below (lines 1740 ff.):
> 
>       LInstruction* result =
>           DefineAsRegister(new(zone()) LLoadKeyedFixed(elements, key, temp));
>       if (instr->RequiresHoleCheck() ||
>           (instr->hole_mode == ... && info()->IsStub()) {
>         result = AssignEnvironment(result);
>       }
>       return result;

Done, and I made the double case above follow the same protocol...looks nicer.

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-code...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-code...
src/arm64/lithium-codegen-arm64.cc:3626: __ ldr(result, FieldMemOperand(result,
Cell::kValueOffset));
On 2015/04/23 13:40:00, Jakob wrote:
> nit: s/ldr/Ldr/ (always use MacroAssembler instructions on arm64).

Done.

https://codereview.chromium.org/1100083002/diff/100001/src/arm64/lithium-code...
src/arm64/lithium-codegen-arm64.cc:3631: __ bind(&done);
On 2015/04/23 13:40:00, Jakob wrote:
> nit: s/bind/Bind/

Done.

https://codereview.chromium.org/1100083002/diff/100001/src/code-stubs-hydroge...
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/code-stubs-hydroge...
src/code-stubs-hydrogen.cc:582: if (casted_stub()->elements_kind() ==
FAST_HOLEY_ELEMENTS &&
On 2015/04/23 13:40:00, Jakob wrote:
> Why do we need to check for elements kind again? The IC/handler compilers
> already do that when computing the convert_hole_to_undefined flag, no?

Indeed, good catch, fixed.

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen-instructi...
src/hydrogen-instructions.h:6614: kBitsForBaseOffset = 25,
On 2015/04/23 13:40:00, Jakob wrote:
> I think instead of moving |dehoisted| out of this bitfield, I'd rather shrink
> the base offset field to 24 bits. Is there a reason not to do that?

Okay, this is done. I was just being cautious (at the expense of memory :p).

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/hydrogen.cc#newcod...
src/hydrogen.cc:6995: Map* holey_double_elements =
On 2015/04/23 13:40:00, Jakob wrote:
> Suggestion: "bool holey_double_elements = *map == ..."
> That would make subsequent code look a bit cleaner; but I don't feel strongly
> about it.

Done.

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codeg...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:3195: __ mov(result,
isolate()->factory()->array_protector());
On 2015/04/23 13:40:00, Jakob wrote:
> You can collapse this into one instruction:
> 
> __ cmp(Operand::ForCell(isolate()->factory()->array_protector()),
>        Immediate(Smi::FromInt(1)));

Actually I can't do that for a PropertyCell, just a Cell. Cool idea though, I
didn't know about that instruction!

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:3197: Immediate(Smi::FromInt(1)));
On 2015/04/23 13:40:00, Jakob wrote:
> Please factor out the magic 0/1 constants as "static const int
> kArrayProtectorValid = 1" (in class Isolate) or similar.

Done.

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (left):

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32....
src/ia32/lithium-ia32.cc:2232: if ((instr->is_external() ||
instr->is_fixed_typed_array()) ?
On 2015/04/23 13:40:00, Jakob wrote:
> Yo dawg, I heard you like conditions, so I put a conditional into your
> condition, so you can check a condition while you check a condition...
> 
> +1 to cleaning that up! :-)

:D, yeah I was like wha?

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/ia32/lithium-ia32....
src/ia32/lithium-ia32.cc:2232: bool needs_environment;
On 2015/04/23 13:40:00, Jakob wrote:
> We used to have to initialize such variables to keep all compilers happy...
not
> sure if that's still necessary.

I'll try to go without it, hopefully it'll work.

https://codereview.chromium.org/1100083002/diff/100001/src/ic/ic-compiler.cc
File src/ic/ic-compiler.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/ic/ic-compiler.cc#...
src/ic/ic-compiler.cc:115: bool convert_hole_to_undefined =
On 2015/04/23 13:40:00, Jakob wrote:
> Checking for a pristine array prototype is pointless here, because this is
> compiled so early that it's pristine anyway, and the stub is doing the check,
> right? Maybe add a comment about the latter, roughly:
> // No need to check for an elements-free prototype chain here, the generated
> // stub code needs to check that dynamically anyway.

That's right, and good idea to comment that. Done.

https://codereview.chromium.org/1100083002/diff/100001/src/mips/lithium-codeg...
File src/mips/lithium-codegen-mips.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/mips/lithium-codeg...
src/mips/lithium-codegen-mips.cc:3295: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
On 2015/04/23 16:24:37, balazs.kilvady wrote:
> The result register should be used instead of scratch in LoadRoot like in
> src/arm64/lithium-codegen-arm64.cc.

Done.

https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-cod...
File src/mips64/lithium-codegen-mips64.cc (right):

https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-cod...
src/mips64/lithium-codegen-mips64.cc:3344: __ LoadRoot(scratch,
Heap::kArrayProtectorRootIndex);
On 2015/04/23 16:24:37, balazs.kilvady wrote:
> The result register should be used instead of scratch in LoadRoot like in
> src/arm64/lithium-codegen-arm64.cc.

Done.

https://codereview.chromium.org/1100083002/diff/100001/src/mips64/lithium-cod...
src/mips64/lithium-codegen-mips64.cc:3345: __ lw(scratch,
FieldMemOperand(result, Cell::kValueOffset));
On 2015/04/23 15:34:23, paul.l... wrote:
> nit: This should probably be 'ld'. I say 'probably' since in general values
are
> 64-bit. But you are using just a bit flag here, so it's fine (as long as we
> never go to big-endian mips64, which we have no plans for :) 
> 
> It might be worth a comment here that you are using just LS bit(s) of value.

Done.

[mvstanton, 2015-04-27 08:20:17]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2015-04-27 08:20:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
abigail.schenk6@gmail.com, jkummerow@chromium.org, paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1100083002/#ps140001
(title: "REBASE and fix arm64 compilation.")

[commit-bot: I haz the power, 2015-04-27 08:20:23]

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

[commit-bot: I haz the power, 2015-04-27 08:47:04]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-04-27 08:47:22]

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