https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode1953
src/hydrogen-instructions.cc:1953: InductionVariableData*
InductionVariableData::New(HPhi* phi) {
On 2013/06/24 14:03:30, titzer wrote:
This method really isn't a "new" or constructor method, since it can
return
NULL. It needs a more descriptive name.
Done (renamed as "ExaminePhi").
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode1977
src/hydrogen-instructions.cc:1977: HValue** context) {
On 2013/06/24 14:03:30, titzer wrote:
Returning potentially four different values via pointer is getting a
little
awkward here. I think a little data object, either created in the
caller or the
callee, would be cleaner.
Done.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode2171
src/hydrogen-instructions.cc:2171: HValue* phi_operand) {
On 2013/06/24 14:03:30, titzer wrote:
I think this code would be cleaner if all the match failures fall
through to a
single case at the end which returns 0. The name "FindIncrement" or
maybe
"MatchIncrement" might be better.
Done.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode2208
src/hydrogen-instructions.cc:2208: HValue*
InductionVariableData::DiscardOsrValue(HValue* v) {
On 2013/06/24 14:03:30, titzer wrote:
"DiscardOsrValue" implies something is being deleted or removed. Maybe
"IgnoreOsrValue".
Done.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode2232
src/hydrogen-instructions.cc:2232: bool
InductionVariableData::ValidateToken(Token::Value token) {
On 2013/06/24 14:03:30, titzer wrote:
Validate the token how? This is not clear.
Renamed, and added comment.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.h#newcode3054
src/hydrogen-instructions.h:3054: class InductionVariableData;
On 2013/06/24 14:03:30, titzer wrote:
It seems like the only reason that all this code lives in this header
file is so
that you can hang it directly off the HPhi instruction. I think we can
avoid
that entanglement if we have a side table (essentially HashMap<Phi,
InductionVariableData> in the array bounds check elimination class
(which should
be in its own file).
As discussed off line with danno, we can keep the reference in HPhi
(having induction variable info can will be useful elsewhere, for
instance for prefetching in indexed loads).
https://codereview.chromium.org/17568015/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/17568015/diff/1/src/hydrogen.cc#newcode4378
src/hydrogen.cc:4378: class InductionVariableBlocksTable BASE_EMBEDDED {
On 2013/06/24 14:03:30, titzer wrote:
Highly recommend moving this class and related functionality into a
new file
specifically for doing bounds check elimination.
Done.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/17568015/diff/1/src/hydrogen.h#newcode268
src/hydrogen.h:268: bool IsNestedInThisLoop(HBasicBlock* other_block) {
On 2013/06/24 14:03:30, titzer wrote:
This method overloading is just for convenience; I think it's better
just to
have one version and inline this one into its caller(s). Or, you could
inline
the other version into this method and remove the current_loop()
method.
Removed and inlined.
https://codereview.chromium.org/17568015/
--
--
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.
