Hi Benedikt, here are comments. Looking good!
--Michael
https://codereview.chromium.org/61893009/diff/1/src/code-stubs.cc
File src/code-stubs.cc (right):
https://codereview.chromium.org/61893009/diff/1/src/code-stubs.cc#newcode684
src/code-stubs.cc:684: void
NewStringAddStub::PrintBaseName(StringStream* stream) {
Wow, I'm going to add this to some of my own stubs, very nice. :)
https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1495
src/hydrogen.cc:1495: // right now.
I appreciate the detailed comments on this long and tricky method!
Instead of saying "this may be too pessimistic," could you say what you
would prefer to do, and then lament that it's non-portable right now.
https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1558
src/hydrogen.cc:1558:
Add<HConstant>(static_cast<int32_t>(kOneByteDataHintMask))),
These hurt my eyes. :)
How about a helper function that consolidates this kind of typical
bitwise operation for this method. The load of Add, <, >, static_cast,
obscures the overall operation.
https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1612
src/hydrogen.cc:1612:
Add<HConstant>(static_cast<int32_t>(kStringEncodingMask))),
More instances of your typical pattern here, OP1(OP2(hvalue1, hvalue2),
mask).
https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1647
src/hydrogen.cc:1647:
The size operand calculation above is very tricky and very similar to
the block below (under if_onebyte.Else()). Can it be moved into a
function?
https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1655
src/hydrogen.cc:1655: HStoreNamedField* store_map =
AddStoreMapConstant(string, map);
AddStoreMapConstantNoWriteBarrier() again, or as a mode of
AddStoreMapConstant().
https://codereview.chromium.org/61893009/diff/1/src/hydrogen.cc#newcode1698
src/hydrogen.cc:1698: HStoreNamedField* store_map =
AddStoreMapConstant(string, map);
Could you add a variant to AddStoreMapConstant either with a parameter
or a new name (like AddStoreMapConstantNoWriteBarrier) that encapsulates
these? As discussed earlier, this "special case" of skipping write
barriers can be dangerous to program, and it would be nice to
consolidate and better name those instances when we do this.
https://codereview.chromium.org/61893009/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):
https://codereview.chromium.org/61893009/diff/1/src/ia32/full-codegen-ia32.cc#newcode3679
src/ia32/full-codegen-ia32.cc:3679: NewStringAddStub
stub(STRING_ADD_CHECK_BOTH, NOT_TENURED);
Is there a loss of performance here because we don't pretenure according
to the global mode from full codegen any more? As we move to site-based
pretenuring, will you have a version of NewStringAddStub that creates
mementos?
https://codereview.chromium.org/61893009/
--
--
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.
