Looking good. First round of comments, just one real issue about stores
with map
transitions. But this needs dedicated test coverage in an
mjsunit/compiler/load-store-elimination.js file.
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc
File src/hydrogen-load-elimination.cc (right):
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc#newcode39
src/hydrogen-load-elimination.cc:39: class HFieldApprox : public
ZoneObject {
nit: Can we use the full name HFieldApproximation?
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc#newcode44
src/hydrogen-load-elimination.cc:44: HFieldApprox *next_;
nit: Asterisk sticks to the left.
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc#newcode51
src/hydrogen-load-elimination.cc:51: class HLoadElimTable {
nit: Can we use the full name HLoadEliminationTable? Also can we mark
the table as BASE_EMBEDDED?
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc#newcode109
src/hydrogen-load-elimination.cc:109: void Kill(HStoreNamedField* store)
{
This method is never called, let's drop it.
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc#newcode218
src/hydrogen-load-elimination.cc:218: Zone* zone_;
nit: Make the field private. Maybe even make the internal methods like
FieldOf() and KillFieldInternal() private.
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc#newcode249
src/hydrogen-load-elimination.cc:249: case HValue::kLoadNamedField: {
style: Indent cases by one level.
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.cc#newcode271
src/hydrogen-load-elimination.cc:271: HValue* result =
table.store(store);
An HStoreNamedField with a transition also changes what is stored in
field 0, because it performs an implicit map change. This must be
reflected in the table accordingly.
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.h
File src/hydrogen-load-elimination.h (right):
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.h#newcode28
src/hydrogen-load-elimination.h:28: #ifndef V8_LOAD_ELIMINATION_H_
nit: s/V8_LOAD_ELIMINATION_H_/V8_HYDROGEN_LOAD_ELIMINATION_H_/
https://codereview.chromium.org/24117004/diff/1/src/hydrogen-load-elimination.h#newcode42
src/hydrogen-load-elimination.h:42: void EliminateLoads(HBasicBlock*
block);
nit: Can we make EliminateLoads() private?
https://codereview.chromium.org/24117004/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
