Optimize Smi keys for KeyedLoads

Optimize Smi keys for KeyedLoads

Allows KeyeLoad/KeyedStore operations where the key is a Smi to fold the untagging of the key into the element offset calculation.

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

[Michael Starzinger, 2012-07-19 21:02:23]

LGTM (with nits).

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

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2786: __ add(scratch, elements, Operand(key, LSL,
kPointerSizeLog2 - 1));
Can we use kSmiTagSize here?

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2818: ? (element_size_shift - 1) :
element_size_shift;
Likewise.

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2874: return MemOperand(base, key, LSR, 1);
Likewise.

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2882: return MemOperand(base, scratch0(), LSR,
1);
Likewise.

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:2904: ? (element_size_shift - 1) :
element_size_shift;
Likewise.

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/arm/lithium-co...
src/arm/lithium-codegen-arm.cc:3844: __ add(scratch, elements, Operand(key, LSL,
kPointerSizeLog2 - 1));
Likewise.

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

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:2706: if (key_representation.IsTagged() &&
shift_size >= 1) {
Can we also make this condition use !ExternalArrayOpRequiresTemp(...) and
instead ASSERT(shift_size >= kSmiTagSize)?

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:2707: shift_size -= 1;
Can we use kSmiTagSize here?

[danno, 2012-07-20 09:51:57]

Feedback addressed, landing.

http://codereview.chromium.org/10735020/diff/21001/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/10735020/diff/21001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:2786: __ add(scratch, elements, Operand(key, LSL,
kPointerSizeLog2 - 1));
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Can we use kSmiTagSize here?

Done.

http://codereview.chromium.org/10735020/diff/21001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:2818: ? (element_size_shift - 1) :
element_size_shift;
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/10735020/diff/21001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:2874: return MemOperand(base, key, LSR, 1);
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/10735020/diff/21001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:2882: return MemOperand(base, scratch0(), LSR,
1);
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/10735020/diff/21001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:2904: ? (element_size_shift - 1) :
element_size_shift;
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/10735020/diff/21001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:3844: __ add(scratch, elements, Operand(key, LSL,
kPointerSizeLog2 - 1));
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/10735020/diff/21001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/10735020/diff/21001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:2706: if (key_representation.IsTagged() &&
shift_size >= 1) {
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Can we also make this condition use !ExternalArrayOpRequiresTemp(...) and
> instead ASSERT(shift_size >= kSmiTagSize)?

Done.

http://codereview.chromium.org/10735020/diff/21001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:2707: shift_size -= 1;
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Can we use kSmiTagSize here?

Done.

[danno, 2012-07-20 10:40:27]

http://codereview.chromium.org/10735020/diff/21001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/10735020/diff/21001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:2706: if (key_representation.IsTagged() &&
shift_size >= 1) {
On 2012/07/19 21:02:23, Michael Starzinger wrote:
> Can we also make this condition use !ExternalArrayOpRequiresTemp(...) and
> instead ASSERT(shift_size >= kSmiTagSize)?

Actually, it turns out that ExternalArrayOpRequiresTemp test something
difference, and one is not replaceable by the other, so I'm leaving the code
as-is.

[Michael Starzinger, 2012-07-20 10:50:25]

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

https://chromiumcodereview.appspot.com/10735020/diff/21001/src/ia32/lithium-c...
src/ia32/lithium-codegen-ia32.cc:2706: if (key_representation.IsTagged() &&
shift_size >= 1) {
On 2012/07/20 10:40:27, danno wrote:
> On 2012/07/19 21:02:23, Michael Starzinger wrote:
> > Can we also make this condition use !ExternalArrayOpRequiresTemp(...) and
> > instead ASSERT(shift_size >= kSmiTagSize)?
> 
> Actually, it turns out that ExternalArrayOpRequiresTemp test something
> difference, and one is not replaceable by the other, so I'm leaving the code
> as-is.

I see, OK. Just use the kSmiTagSize constant.