These are the minimal changes to fix all known performance regressions.
The optimistic hoisting is actually necessary to overcome the "GVN identity"
problem (and is not harmful, at least so far).
Of course I intend to set the idefs flag to false before committing (and
turn it
on in a separate cl).
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")
On 2013/04/17 11:34:15, Jakob wrote:
intentional?
Yes and no.
For now it is on for convenience but I intend to switch it off before
committing and turn it on in a separate CL.
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;
On 2013/04/17 11:34:15, Jakob wrote:
why not just "int direction = -decomposition.scale();"?
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode207
src/hydrogen-instructions.cc:207: &&
decomposition.base()->IsRelationTrue(
On 2013/04/17 11:34:15, Jakob wrote:
nit: '&&' on the previous line please
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode208
src/hydrogen-instructions.cc:208: relation, other, 0, 0)) {
On 2013/04/17 11:34:15, Jakob wrote:
nit: fits on last line?
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1033
src/hydrogen-instructions.cc:1033: : HUnaryOperation(phi), phi_(phi),
relation_(relation),
On 2013/04/17 11:34:15, Jakob wrote:
nit: one line per initializer, please
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1049
src/hydrogen-instructions.cc:1049: ==
context->upper_bound()->ActualValue()) {
On 2013/04/17 11:34:15, Jakob wrote:
nit: operator on the previous line
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1077
src/hydrogen-instructions.cc:1077: if (candidate->IsNumericConstraint())
{
On 2013/04/17 11:34:15, Jakob wrote:
for consistency with code below, I'd transform this into:
if (!candidate->IsNumericConstraint()) continue;
Done.
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;
On 2013/04/17 11:34:15, Jakob wrote:
nit: operator on the previous line
Done.
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;
On 2013/04/17 11:34:15, Jakob wrote:
nit: operator on the previous line
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1096
src/hydrogen-instructions.cc:1096: ||
induction_limit->IsInteger32Constant())) {
On 2013/04/17 11:34:15, Jakob wrote:
nit: operator on the previous line
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1160
src/hydrogen-instructions.cc:1160: } else if
(context->upper_bound_guarantee() != NULL &&
On 2013/04/17 11:34:15, Jakob wrote:
I think the "else" needs to be removed.
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode1160
src/hydrogen-instructions.cc:1160: } else if
(context->upper_bound_guarantee() != NULL &&
On 2013/04/17 11:34:15, Jakob wrote:
I think the "else" needs to be removed.
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode3229
src/hydrogen-instructions.cc:3229: &&
delta->IsRelationTrue(NumericRelation::Ge(),
On 2013/04/17 11:34:15, Jakob wrote:
nit: operator on the previous line
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.cc#newcode3235
src/hydrogen-instructions.cc:3235: &&
delta->IsRelationTrue(NumericRelation::Le(),
On 2013/04/17 11:34:15, Jakob wrote:
nit: operator on the previous line
Done.
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);
On 2013/04/17 11:34:15, Jakob wrote:
I think "other_block" either needs a comment or a more descriptive
name.
"jump_target_when_false"?
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode1505
src/hydrogen-instructions.h:1505: : relation_(relation),
other_block_(other_block) {
On 2013/04/17 11:34:15, Jakob wrote:
nit: indentation
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode3362
src/hydrogen-instructions.h:3362: &&
related_value->IsInteger32Constant()) {
On 2013/04/17 11:34:15, Jakob wrote:
nit: we're pretty consistent about putting binary operators at the end
of the
first line.
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen-instructions.h#newcode4641
src/hydrogen-instructions.h:4641: && right()->GetInteger32Constant() <=
1) {
On 2013/04/17 11:34:15, Jakob wrote:
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.
Done.
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.
On 2013/04/17 11:34:15, Jakob wrote:
s/>>/<</ ?
Done.
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) {
On 2013/04/17 11:34:15, Jakob wrote:
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.
Done.
https://codereview.chromium.org/14046017/diff/1/src/hydrogen.cc#newcode3447
src/hydrogen.cc:3447: if (current->IsThrow()) {
On 2013/04/17 11:34:15, Jakob wrote:
Let's move this into the block ordering phase where ProcessEdge is
called too.
Done.
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) {
On 2013/04/17 11:34:15, Jakob wrote:
I'd rename this to IsNestedInThisLoop (or just IsNested).
Done.
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.
