Add Crankshaft support for smi-only elements

Add Crankshaft support for smi-only elements

Committed: http://code.google.com/p/v8/source/detail?r=9426

[Jakob Kummerow, 2011-09-23 08:04:18]

PTAL.

[danno, 2011-09-23 09:05:24]

http://codereview.chromium.org/8002019/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/8002019/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:3737: ValueType value_type = GENERIC_VALUE)
Why not just use ElementKind here?

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode3342
src/hydrogen.cc:3342: HBasicBlock* store_fast = graph()->CreateBasicBlock();
Wow, cool, you went for bonus points here (I don't know if this is a perf
bottneck or not). It's probably a good idea to include a unit test that
explicitly triggers this logic, since it is now more than a one-liner.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4007
src/hydrogen.cc:4007: HInstruction*
HGraphBuilder::BuildFastElementAccess(HValue* elements,
I find it a little asymmetric that BuildFirstElementAccess adds the SmiCheck,
but not the last one. Maybe instead it should always to the AddInstruction for
the result instruction, and return a HValue* (or NULL for stores)?

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4028
src/hydrogen.cc:4028: return new(zone()) HLoadKeyedFastElement(elements,
checked_key);
If you're refactoring anyway, perhaps you might want to kill the arrow
anti-pattern?
http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4069
src/hydrogen.cc:4069: ASSERT(fast_smi_only_elements || fast_elements ||
fast_double_elements);
Perhaps add predicates to the map that test for multiple values as we did for
Object's HasXXXXElements, like  fast_type_elements()?

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4158
src/hydrogen.cc:4158: // When the |object| has FAST_SMI_ONLY_ELEMENTS, chances
are we'll
stype nit: passive voice instead of "we"?

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4168
src/hydrogen.cc:4168: // TODO(jkummerow): We could get around the need for these
two blocks
stype nit: passive voice instead of "we"?

[danno, 2011-09-23 09:07:52]

a little clarification

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4007
src/hydrogen.cc:4007: HInstruction*
HGraphBuilder::BuildFastElementAccess(HValue* elements,
"not last last one" should read "the returned instruction"
On 2011/09/23 09:05:24, danno wrote:
> I find it a little asymmetric that BuildFirstElementAccess adds the SmiCheck,
> but not the last one. Maybe instead it should always to the AddInstruction for
> the result instruction, and return a HValue* (or NULL for stores)?

[Jakob Kummerow, 2011-09-26 11:30:32]

Here you go, please take another look.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/8002019/diff/1/src/hydrogen-instructions.h#new...
src/hydrogen-instructions.h:3737: ValueType value_type = GENERIC_VALUE)
On 2011/09/23 09:05:24, danno wrote:
> Why not just use ElementKind here?

Done.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode3342
src/hydrogen.cc:3342: HBasicBlock* store_fast = graph()->CreateBasicBlock();
On 2011/09/23 09:05:24, danno wrote:
> Wow, cool, you went for bonus points here (I don't know if this is a perf
> bottneck or not). It's probably a good idea to include a unit test that
> explicitly triggers this logic, since it is now more than a one-liner.

Done.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4007
src/hydrogen.cc:4007: HInstruction*
HGraphBuilder::BuildFastElementAccess(HValue* elements,
On 2011/09/23 09:07:52, danno wrote:
> "not last last one" should read "the returned instruction"
> On 2011/09/23 09:05:24, danno wrote:
> > I find it a little asymmetric that BuildFirstElementAccess adds the
SmiCheck,
> > but not the last one. Maybe instead it should always to the AddInstruction
for
> > the result instruction, and return a HValue* (or NULL for stores)?
> 

It's currently consistent with a bunch of other methods in hydrogen.cc. I can
refactor it everywhere, but would prefer to do that in a separate CL as it's
unrelated.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4028
src/hydrogen.cc:4028: return new(zone()) HLoadKeyedFastElement(elements,
checked_key);
On 2011/09/23 09:05:24, danno wrote:
> If you're refactoring anyway, perhaps you might want to kill the arrow
> anti-pattern?
> http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

Done. Removed the unnecessary outer "else" and added comments to the other
"else" blocks.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4069
src/hydrogen.cc:4069: ASSERT(fast_smi_only_elements || fast_elements ||
fast_double_elements);
On 2011/09/23 09:05:24, danno wrote:
> Perhaps add predicates to the map that test for multiple values as we did for
> Object's HasXXXXElements, like  fast_type_elements()?

I'd prefer not to do that, for two reasons:
(1) This ASSERT is only used once, so we wouldn't save any code by introducing a
predicate
(2) It's as much for documentation ("at this point in the code, X is handled")
as for asserting correctness, so I like to keep the listing of cases explicit to
the reader.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4158
src/hydrogen.cc:4158: // When the |object| has FAST_SMI_ONLY_ELEMENTS, chances
are we'll
On 2011/09/23 09:05:24, danno wrote:
> stype nit: passive voice instead of "we"?

Done -- both comment and the code it referred to are obsolete, I have removed
them.

http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4168
src/hydrogen.cc:4168: // TODO(jkummerow): We could get around the need for these
two blocks
On 2011/09/23 09:05:24, danno wrote:
> stype nit: passive voice instead of "we"?

Done.

[danno, 2011-09-26 11:55:23]

lgtm