Reviewers: rossberg, ulan,
Message:
Please take another look, much sub-functionality has already been landed in
other CLs.
Ulan, can you please take a look at the dependency changes?
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs-hydrogen.cc#newcode780
src/code-stubs-hydrogen.cc:780: Handle<Object> hole =
Handle<Object>(isolate()->heap()->the_hole_value(),
On 2013/06/25 10:47:38, rossberg wrote:
Nit: can use constructor init syntax to avoid the duplicate type
Done.
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode527
src/code-stubs.h:527: explicit StoreGlobalStub(StrictModeFlag
strict_mode,
On 2013/06/25 10:47:38, rossberg wrote:
The 'explicit' is unnecessary here.
Also, parameters fit on one line.
Done.
https://codereview.chromium.org/16925008/diff/39001/src/code-stubs.h#newcode542
src/code-stubs.h:542: return bit_field_;
On 2013/06/25 10:47:38, rossberg wrote:
Nit: I believe this fits on the prev line
Done.
https://codereview.chromium.org/16925008/diff/39001/src/heap.h
File src/heap.h (right):
https://codereview.chromium.org/16925008/diff/39001/src/heap.h#newcode264
src/heap.h:264: V(cell_value_string, "%call_value")
\
On 2013/06/25 10:47:38, rossberg wrote:
Typo? %cell_value
Done.
https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/hydrogen.cc#newcode6561
src/hydrogen.cc:6561: AddSoftDeoptimize(MUST_EMIT_SOFT_DEOPT);
On 2013/06/25 10:47:38, rossberg wrote:
I'm a bit at a loss why this flag is needed.
Most soft deopts are only for performance. Without the deopt, we'll just
use generic ICs, those can be removed. However, some soft deopts protect
against incorrect code, those cant be removed. FLAG_always_opt removes
the former kind of soft deopts, but it musn't remove the latter kind.
https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-resolver-ia32.cc
File src/ia32/lithium-gap-resolver-ia32.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-resolver-ia32.cc#newcode320
src/ia32/lithium-gap-resolver-ia32.cc:320: int32_t upper =
static_cast<int32_t>(int_val >> (kBitsPerInt));
On 2013/06/25 10:47:38, rossberg wrote:
Nit: redundant parens
Done.
https://codereview.chromium.org/16925008/diff/39001/src/ia32/lithium-gap-resolver-ia32.cc#newcode336
src/ia32/lithium-gap-resolver-ia32.cc:336: cgen_->PopX87();
On 2013/06/25 10:47:38, rossberg wrote:
Hm, why this?
This is an artifact of how we use the x87 FP stack on non-SSE machines.
The invariant is that there can only be a single value on the FP stack
at once. If there is a value on the stack, it is no longer live, but the
register allocation doesn't do anything special for live ranges that
end, so we much handle that explicitly by popping a value of the FP
stack when the next live value begins.
This is kind of an idiom used here and elsewhere in Crankshaft non-SSE2
FP code.
https://codereview.chromium.org/16925008/diff/39001/src/ic.cc
File src/ic.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/ic.cc#newcode2098
src/ic.cc:2098: Handle<Object> value) {
On 2013/06/25 10:47:38, rossberg wrote:
Where is this parameter used?
In the non-Keyed case only. This is a virtual method.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10132
src/objects.cc:10132: if (result == NULL) return NULL;
On 2013/06/25 10:47:38, rossberg wrote:
Nit: could avoid the extra return using ?:
Done.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode10218
src/objects.cc:10218: Cell* replace_with) {
On 2013/06/25 10:47:38, rossberg wrote:
Nit: this fits on the previous line
Done.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15823
src/objects.cc:15823: Type* PropertyCell::UnionType(Object* value) {
On 2013/06/25 10:47:38, rossberg wrote:
I'm not sure I fully understand the logic of this function.
Intuitively, it
seems more complicated than it should be (I had to draw a state
transition
diagram to follow it :) ).
Also, 'UnionType' is a bit of an odd name, since it never computes a
proper
union. And it doesn't make it particularly obvious that it has the
side effect
of causing deopts. How about something like 'UpdateType'?
Done.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15829
src/objects.cc:15829: if (new_type->Is(old_type) && !value->IsTheHole())
{
On 2013/06/25 10:47:38, rossberg wrote:
There is repeated special-casing for the hole. Maybe that can be
avoided by
initialising new_type as:
Handle<Type> new_type(value->IsTheHole() ? Type::Any() :
Type::Constant(value_handle, isolate), isolate);
Done.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15836
src/objects.cc:15836: if (*old_type == Type::None()) {
On 2013/06/25 10:47:38, rossberg wrote:
Please don't rely on pointer equality for types. This should be
"old_type->Is(Type::None())".
Done.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15837
src/objects.cc:15837: if (new_type->IsConstant() && !value->IsTheHole())
{
On 2013/06/25 10:47:38, rossberg wrote:
Given that new_type is initialised to a Constant above, the first part
of the
condition seems redundant. Moreover, if you initialise new_type as I
suggested
above, I believe you can return new_type unconditionally.
Done.
https://codereview.chromium.org/16925008/diff/39001/src/objects.cc#newcode15840
src/objects.cc:15840: } else if (old_type->IsConstant() &&
old_type->AsConstant()->IsUndefined()) {
On 2013/06/25 10:47:38, rossberg wrote:
I think you can just as well test "old_type->Is(Type::Undefined())"
for this.
Also, IIUC, you allow overriding undefined with the hole here.
Done.
https://codereview.chromium.org/16925008/diff/39001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode4693
src/objects.h:4693: void ReplaceNthObject(int n, Map* match_map, Object*
replace_with);
On 2013/06/25 10:47:38, rossberg wrote:
As we discussed earlier, this seems rather fragile. Is the plan still
to have
an explicit notion of parameters eventually?
Yes, I very much plan to clean this up, but it seems out of scope of
this CL. It seemed like too much prerequisite work to make this change
dependent on.
https://codereview.chromium.org/16925008/diff/39001/src/objects.h#newcode8597
src/objects.h:8597: void set_value_infer_type(Object* value,
On 2013/06/25 10:47:38, rossberg wrote:
This function can also trigger deopts, so I'd prefer not giving it a
seemingly
innocent lower-case name.
Done.
https://codereview.chromium.org/16925008/diff/39001/src/types.cc
File src/types.cc (right):
https://codereview.chromium.org/16925008/diff/39001/src/types.cc#newcode133
src/types.cc:133: if (value->IsTrue() || value->IsFalse()) return
kBoolean;
On 2013/06/25 10:47:38, rossberg wrote:
You might want to add a case sending the hole to type kAny here. So
far, I've
basically ignored the hole, because it's an internal type.
Done.
Description:
Generate StoreGlobal stubs with Hydrogen
Please review this at https://codereview.chromium.org/16925008/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files:
M src/arm/code-stubs-arm.cc
M src/arm/lithium-gap-resolver-arm.cc
M src/ast.cc
M src/code-stubs-hydrogen.cc
M src/code-stubs.h
M src/code-stubs.cc
M src/heap.h
M src/heap.cc
M src/hydrogen-instructions.h
M src/hydrogen-instructions.cc
M src/hydrogen.h
M src/hydrogen.cc
M src/ia32/code-stubs-ia32.cc
M src/ia32/lithium-codegen-ia32.h
M src/ia32/lithium-gap-resolver-ia32.cc
M src/ic.h
M src/ic.cc
M src/objects-inl.h
M src/objects.h
M src/objects.cc
M src/runtime.cc
M src/stub-cache.h
M src/stub-cache.cc
M src/x64/code-stubs-x64.cc
M src/x64/lithium-gap-resolver-x64.cc
M test/mjsunit/regress/regress-2618.js
--
--
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.
