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#newcode3737
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.
http://codereview.chromium.org/8002019/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
