[hydrogen] don't bailout assignments to consts

[hydrogen] don't bailout assignments to consts

If constant variable is allocated in CONTEXT

Patch by Fedor Indutny <fedor.indutny@gmail.com>;.

BUG=
TEST=
R=vegorov@chromium.org
Committed: http://code.google.com/p/v8/source/detail?r=10244

[fschneider, 2011-12-08 13:01:30]

Approach looks OK. Could you add a unit test that makes sure that it triggers
the optimizing compiler?

http://codereview.chromium.org/8857001/diff/2001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/8857001/diff/2001/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:2316: if (instr->needs_is_hole_check()) {
Maybe rename this to instr->RequiresHoleCheck() to be consistent with other
instructions that have a similar predicate.

(also applies to ia32 and x64 files)

http://codereview.chromium.org/8857001/diff/2001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8857001/diff/2001/src/hydrogen.cc#newcode3965
src/hydrogen.cc:3965: if (var->is_const_mode()) {
CONST_HARMONY has different semantics from CONST. There is another change under
review to deal with harmony-const (http://codereview.chromium.org/8820015/), so
you should leave it out in this change and only handle plain CONST.

[indutny, 2011-12-08 19:05:22]

Everything fixed, and I noticed that it was anyway bailouting on
self-referencing named function, because of VisitVariableProxy for CONST
variable in CONTEXT. Do we need this bailout anymore?

And looks like my test is failing on new test: "illegall access". This is
because arm platform is using Runtime::kLoadContextSlot helper with wrong
arguments, and I'm not sure if reason of that fail is my patch

[indutny, 2011-12-09 08:59:34]

Sorry, bad grammar, never write replies too late :) hope everything is clear
from my message anyway.

[fschneider, 2011-12-09 09:37:53]

http://codereview.chromium.org/8857001/diff/6002/src/hydrogen.cc
File src/hydrogen.cc (left):

http://codereview.chromium.org/8857001/diff/6002/src/hydrogen.cc#oldcode3292
src/hydrogen.cc:3292: return Bailout("reference to const context slot");
In order to get rid of this bailout we also additional code for for
HLoadContextSlot to return undefined if we find the hole-value:

1. Load value
2. Check if value is the_hole_value.
3. If no, return value.
4. If yes, return undefined_value.

http://codereview.chromium.org/8857001/diff/6002/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8857001/diff/6002/src/hydrogen.cc#newcode4034
src/hydrogen.cc:4034: if (var->is_const_mode()) {
Here I would write

if (var->mode() == CONST)

to handle the CONST case only.

[indutny, 2011-12-09 10:19:04]

Updated with fixes

[fschneider, 2011-12-09 13:14:14]

On 2011/12/09 10:19:04, indutny wrote:
> Updated with fixes

The new test still seems to fail on ARM. Do you have a fix for it?

[indutny, 2011-12-09 13:38:33]

Not right now, but going to investigate...

[indutny, 2011-12-09 19:09:07]

Fixed, I was checking value, not target in StoreContextSlot.

[indutny, 2011-12-12 09:24:17]

any status update?

[fschneider, 2011-12-12 09:55:55]

Almost there - there is a conflicting change, so you'll need some adjustments in
your code now. Otherwise looks good.

http://codereview.chromium.org/8857001/diff/12001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/8857001/diff/12001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:3452: kIsHoleCheck
Please rebase your change. It seems to conflict with r10220 which also
introduces harmony-related changes (handling CONST_HARMONY etc.).

Basically you need to change this to have 3 modes here. (no check, const-harmony
hole-check (deoptimizes) and const hole-check (returns undefined).

The HStoreContextSlow will also need to be changed accordingly.

[indutny, 2011-12-12 11:47:23]

Done!

[Steven, 2011-12-12 13:12:44]

Thanks for submitting this CL. I have some drive by comments below.

http://codereview.chromium.org/8857001/diff/17002/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/8857001/diff/17002/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:2311: } else {
You can do better here and use a conditional move instead:
__ mov(result, Operand(factory()->undefined_value()), LeaveCC, eq);

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:3549: return mode_ == kCheckIgnoreAssignment;
This should better be mode_ != kNoCheck?

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode3821
src/hydrogen.cc:3821: Bind(var, Top());
mjsunit/compiler/regress-const.js fails because of a compound assignment to a
stack allocated CONST variable. You should also skip the assignment here.

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode3845
src/hydrogen.cc:3845: case CONST_HARMONY:
You can put an UNREACHABLE() in the CONST_HARMONY case, because we statically
check for this case and throw an early syntax error.

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode4035
src/hydrogen.cc:4035: case CONST_HARMONY:
Here also CONST_HARMONY should be UNREACHABLE().

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode4047
src/hydrogen.cc:4047: ASSERT(expr->op() == Token::INIT_CONST_HARMONY ||
Here CONST_HARMONY can be moved to the HStoreContextSlot::kNoCheck group,
because in contrast to CONST repeated initializations (for example in a loop)
are not possible with CONST_HARMONY.

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen.cc#newcode5624
src/hydrogen.cc:5624: return Bailout("unsupported count operation with const");
It should be trivial to also remove this bailout and do the same thing as in the
CompoundAssignments and ignore the assignment to CONST. Could you also add
simple test cases, that test that crankshafted CompoundAssignments and
CountOperations have the expected behaviour for context allocated CONST
bindings?

http://codereview.chromium.org/8857001/diff/17002/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/8857001/diff/17002/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2176: 
Those blank lines do not help readability. Could you remove them please?

http://codereview.chromium.org/8857001/diff/17002/src/mips/lithium-codegen-mi...
File src/mips/lithium-codegen-mips.cc (right):

http://codereview.chromium.org/8857001/diff/17002/src/mips/lithium-codegen-mi...
src/mips/lithium-codegen-mips.cc:2188: } else {
You can also introduce a conditional move here. Compare
LCodeGen::DoArgumentsElements on arm and mips to see how to do it.

[indutny, 2011-12-12 13:59:05]

Done.

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/8857001/diff/17002/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:3549: return mode_ == kCheckIgnoreAssignment;
oh, sorry. this was made by mistake (ok in LoadContextSlot)

[fschneider, 2011-12-13 16:15:00]

http://codereview.chromium.org/8857001/diff/19008/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8857001/diff/19008/src/hydrogen.cc#newcode3853
src/hydrogen.cc:3853: UNREACHABLE();
Maybe a short comment that this is checked statically and can't occur therefore.

http://codereview.chromium.org/8857001/diff/19008/src/hydrogen.cc#newcode4044
src/hydrogen.cc:4044: UNREACHABLE();
Also a short comment for the case here that this is already checked.

http://codereview.chromium.org/8857001/diff/19008/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/8857001/diff/19008/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:2077: 
Remove extra blank line.

http://codereview.chromium.org/8857001/diff/19008/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:2079: 
Also no need for blank line here.

[indutny, 2011-12-13 16:49:28]

Done

[fschneider, 2011-12-13 16:55:10]

LGTM.

I'll land the patch.