On 2013/09/12 20:04:25, Michael Starzinger wrote:
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.
Done.
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?
Done.
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.
Done.
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?
Done.
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.
Done.
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.
Done.
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.
Done.
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.
Done. I've also improved the heuristic for telling when the stored value is
the
same (using HValue::Equals()).
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_/
Done.
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?
Done.
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.
