https://codereview.chromium.org/14046017/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):
https://codereview.chromium.org/14046017/diff/1/src/flag-definitions.h#newcode221
src/flag-definitions.h:221: DEFINE_bool(idefs, true, "use informative
definitions")
intentional?
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode205
src/hydrogen-instructions.cc:205: int direction = decomposition.scale()
0 ? -1 : 1;
why not just "int direction = -decomposition.scale();"?
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode207
src/hydrogen-instructions.cc:207: &&
decomposition.base()->IsRelationTrue(
nit: '&&' on the previous line please
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode208
src/hydrogen-instructions.cc:208: relation, other, 0, 0)) {
nit: fits on last line?
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1033
src/hydrogen-instructions.cc:1033: : HUnaryOperation(phi), phi_(phi),
relation_(relation),
nit: one line per initializer, please
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1049
src/hydrogen-instructions.cc:1049: ==
context->upper_bound()->ActualValue()) {
nit: operator on the previous line
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1061
src/hydrogen-instructions.cc:1061: &&
!block()->graph()->use_optimistic_licm()) return;
If it doesn't make a significant difference, I'd rather be careful than
optimistic here.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1063
src/hydrogen-instructions.cc:1063: bool
starting_loop_is_nested_in_my_loop = false;
we can make this loop a little simpler:
// See if starting_loop is nested in my_loop.
while (starting_loop != my_loop) {
starting_loop = starting_loop->parent_loop();
if (starting_loop == NULL) return;
}
or you could just call "my_loop->IsInThisLoop(starting_loop);"
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1077
src/hydrogen-instructions.cc:1077: if (candidate->IsNumericConstraint())
{
for consistency with code below, I'd transform this into:
if (!candidate->IsNumericConstraint()) continue;
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1090
src/hydrogen-instructions.cc:1090: &&
!induction_limit->block()->Dominates(my_pre_header)) continue;
nit: operator on the previous line
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1092
src/hydrogen-instructions.cc:1092: &&
!upper_bound->block()->Dominates(my_pre_header)) continue;
nit: operator on the previous line
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1096
src/hydrogen-instructions.cc:1096: ||
induction_limit->IsInteger32Constant())) {
nit: operator on the previous line
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1160
src/hydrogen-instructions.cc:1160: } else if
(context->upper_bound_guarantee() != NULL &&
I think the "else" needs to be removed.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode3229
src/hydrogen-instructions.cc:3229: &&
delta->IsRelationTrue(NumericRelation::Ge(),
nit: operator on the previous line
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode3235
src/hydrogen-instructions.cc:3235: &&
delta->IsRelationTrue(NumericRelation::Le(),
nit: operator on the previous line
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode1466
src/hydrogen-instructions.h:1466: HBasicBlock* other_block = NULL);
I think "other_block" either needs a comment or a more descriptive name.
"jump_target_when_false"?
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode1505
src/hydrogen-instructions.h:1505: : relation_(relation),
other_block_(other_block) {
nit: indentation
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode3362
src/hydrogen-instructions.h:3362: &&
related_value->IsInteger32Constant()) {
nit: we're pretty consistent about putting binary operators at the end
of the first line.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode4641
src/hydrogen-instructions.h:4641: && right()->GetInteger32Constant() <=
1) {
As discussed, I think you need an additional check here that
right()->GetInteger32Constant >= 0.
Please check right shifts for the same issue.
And a nit: '&&' on the previous line please.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode4644
src/hydrogen-instructions.h:4644: // like ((base + offset) >> scale)
with one single decomposition.
s/>>/<</ ?
https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc#newcode455
src/hydrogen.cc:455: while (current_loop != NULL) {
Please add a comment saying that when we support try..catch in
crankshaft, we need another condition here: break the while-loop when
encountering the catch block. From a correctness PoV we're safe anyway,
but unnecessarily marking loops as having additional exits could prevent
BCE.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc#newcode3447
src/hydrogen.cc:3447: if (current->IsThrow()) {
Let's move this into the block ordering phase where ProcessEdge is
called too.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/14046017/diff/1/src/hydrogen.h#newcode251
src/hydrogen.h:251: bool IsInThisLoop(HLoopInformation* other) {
I'd rename this to IsNestedInThisLoop (or just IsNested).
https://codereview.chromium.org/14046017/
--
--
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.
