Hi Ben,
Looking pretty cool. Some comments around kChangesMaps and stat reporting.
--Michael
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc
File src/hydrogen-check-elimination.cc (right):
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode47
src/hydrogen-check-elimination.cc:47: static const int
kMaxTrackedObjects = 10;
Could this be public static inside the HCheckTable?
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode100
src/hydrogen-check-elimination.cc:100: // If the instruction changes
maps uncontrollably, kill the whole town.
LOL! But a more sober comment for checking in, pleez.
Also, should these flags be checked for the instructions in the cases
above too?
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode120
src/hydrogen-check-elimination.cc:120: new_entry->object_ =
old_entry->object_;
Can you explain here why you don't copy the checks as well as
object_/maps_?
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode251
src/hydrogen-check-elimination.cc:251: Kill(instr->object());
I don't like the combination of action (Kill()) and notice of unexpected
state (UNREACHABLE()). Better to assert that the situation is completely
unexpected or desired.
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode261
src/hydrogen-check-elimination.cc:261: if (maps->size() == 1)
INC_STAT(compares_true_);
It looks like this function just serves to update reporting stats, at
least until the TODOs are addressed. Could you put stat collection
behind a flag (instead of implicit with DEBUG), and avoid going deep
into code paths if stat reporting is turned off? So this function would
begin with
if (!FLAG_check_elimination_stats) return;
...
// TODO(...
(those TODOs point the way that soon an implementation will make it
worthwhile to come into this function).
The INC_STAT macro will check the flag.
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode333
src/hydrogen-check-elimination.cc:333: cursor_ = size_; // Move cursor
to end.
Do performance concerns justify the complexity? How about just use the
tmp_entries every time as a serialization from cursor_ to the oldest
entry?
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode398
src/hydrogen-check-elimination.cc:398: int16_t cursor_; // Must be <=
kMaxTrackedObjects
STATIC_ASSERT on sizeof cursor_ and size_ that they hold
kMaxTrackedObjects, since you are already reducing their size.
https://codereview.chromium.org/46883009/diff/1/src/hydrogen-check-elimination.cc#newcode487
src/hydrogen-check-elimination.cc:487: #if DEBUG
Same here with a defined flag instead of DEBUG. I guess the flag is
already FLAG_trace_check_elimination.
https://codereview.chromium.org/46883009/
--
--
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.
