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) {
This method really isn't a "new" or constructor method, since it can
return NULL. It needs a more descriptive name.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode1977
src/hydrogen-instructions.cc:1977: HValue** context) {
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.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode2171
src/hydrogen-instructions.cc:2171: HValue* phi_operand) {
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.
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode2208
src/hydrogen-instructions.cc:2208: HValue*
InductionVariableData::DiscardOsrValue(HValue* v) {
"DiscardOsrValue" implies something is being deleted or removed. Maybe
"IgnoreOsrValue".
https://codereview.chromium.org/17568015/diff/1/src/hydrogen-instructions.cc#newcode2232
src/hydrogen-instructions.cc:2232: bool
InductionVariableData::ValidateToken(Token::Value token) {
Validate the token how? This is not clear.
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;
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).
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 {
Highly recommend moving this class and related functionality into a new
file specifically for doing bounds check elimination.
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) {
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.
https://codereview.chromium.org/17568015/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/17568015/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode4382
src/ia32/lithium-codegen-ia32.cc:4382: void
LCodeGen::ApplyCheckIf(Condition cc, LBoundsCheck* check) {
Is this a remnant of debugging code? What does this do?
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.
