Addressed feedback comments.
Now the only things missing are 1 tiny stack slot on ARM and a similar one
on
MIPS... :-P
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h
File src/builtins.h (right):
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h#newcode160
src/builtins.h:160: V(SetterStubForDeopt, STORE_IC,
MONOMORPHIC, \
On 2012/08/16 17:20:04, Michael Starzinger wrote:
Can we call this "StoreIC_Setter_ForDeopt"?
Done.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc
File src/ia32/deoptimizer-ia32.cc (right):
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode700
src/ia32/deoptimizer-ia32.cc:700: // The receiver and RHS are expected
in registers by the IC, so they don't
On 2012/08/16 17:20:04, Michael Starzinger wrote:
s/IC/StoreIC/
Done here and on other platforms.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode702
src/ia32/deoptimizer-ia32.cc:702: // of 0 instead of 2.
On 2012/08/16 17:20:04, Michael Starzinger wrote:
Drop the "instead of 2" part.
... it's cleaner! ;-) Done here and on other platforms.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode709
src/ia32/deoptimizer-ia32.cc:709: // 1 stack entry for the return
address + 4 stack entries from
On 2012/08/16 17:20:04, Michael Starzinger wrote:
Can we start this sentence with a word, not a number, to make it a
proper
sentence.
Done here and on other platforms.
Also we should really think about moving these constants in
frames-[arch].h at
some point, but that's for a followup CL.
Definitely, but we should then do this for all deoptimizer functions at
once.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode777
src/ia32/deoptimizer-ia32.cc:777: value =
reinterpret_cast<intptr_t>(setter->code());
On 2012/08/16 17:20:04, Michael Starzinger wrote:
I don't think this is quite right. What you want here is the code
object for the
setter stub, not for the JavaScript setter. So this should be
"setter_stub"
instead.
Good catch. Done here and on other platforms.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode790
src/ia32/deoptimizer-ia32.cc:790: // The RHS was part of the artificial
setter stub environment.
On 2012/08/16 17:20:04, Michael Starzinger wrote:
I would use "implicit return value" or "passed value" here instead of
RHS,
because RHS only applies to ordinary assignments, it has little
meaning for
counting or compound assignments.
Done here and on other platforms.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode796
src/ia32/deoptimizer-ia32.cc:796: Code* setter_stub =
On 2012/08/16 17:20:04, Michael Starzinger wrote:
Move this up, you already need if for the code object.
Done here and on other platforms.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cache-ia32.cc#newcode2669
src/ia32/stub-cache-ia32.cc:2669: if (setter.is_null()) {
On 2012/08/16 17:20:04, Michael Starzinger wrote:
Can we flip the condition, thereby the return point would visually be
after the
InvokeFunction part, where it actually needs to be. That's visually
more
intuitive (at least for me).
Good point. Done here and on other platforms.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc
File src/objects.cc (right):
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#newcode8307
src/objects.cc:8307: case Translation::SETTER_STUB_FRAME: {
On 2012/08/16 17:20:04, Michael Starzinger wrote:
Can we make that an own case that doesn't print the height at all?
This makes it
hard to see when we expect the iterator to give us a height and when
not.
Done.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#newcode8312
src/objects.cc:8312: opcode == Translation::SETTER_STUB_FRAME ? 2 :
iterator.Next();
On 2012/08/16 17:20:04, Michael Starzinger wrote:
Actually, height = 0 for the setter stub, but that doesn't matter if
the above
comment is addressed.
Well, it is totally unclear what is *really* meant here: There is a lot
of naming confusion going on, this "height" here is sometimes called
"translation_size", at other places "height" has a different meaning,
compare e.g. LCodeGen::WriteTranslation and
Translation::BeginConstructStubFrame. But as you said, this won't matter
here. :-)
https://chromiumcodereview.appspot.com/10855098/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
