Looks pretty good, I found an issue I think should be fixed first with
HAllocate. I am not quite done looking at hydrogen.cc yet, but other files
are
done.
https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc#newcode1947
src/hydrogen.cc:1947: size, HType::String(), STRING_TYPE,
allocation_mode);
This thing of HAllocate accepting an instance type bothers me, and I
think we've gotten in trouble with it before. Here, we are passing a
deliberately incorrect value half the time. Can you, in a precursor CL,
change HAllocate to not accept an instance type, but instead, factor out
the things it does with instance type to the caller. Ideally, a static
helper function in HAllocate like
static int ComputeInputFlags(InstanceType instance_type)
And the flags that result from that call are OR'd with any other flags
the HAllocate constuctor computes.
The clear_next_map_word_ boolean could actually be a flag.
then, you could write code like:
ASSERT(HAllocate::ComputeInputFlags(STRING_TYPE) ==
HAllocate::ComputeInputFlags(ASCII_STRING_TYPE));
that allows you to safely get away with passing STRING_TYPE for both
cases.
https://codereview.chromium.org/106453003/diff/60001/src/hydrogen.cc#newcode8791
src/hydrogen.cc:8791: ?
HAllocationMode(isolate()->heap()->GetPretenureMode())
Maybe FLAG_allocation_site_pretenuring should be involved in this
decision? So, if the flag is on, then either go with the allocation site
or NOT_TENURED if the site isn't found. If the flag is off, then you
won't be have a site, then the global mode makes sense.
https://codereview.chromium.org/106453003/diff/60001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/106453003/diff/60001/src/ia32/code-stubs-ia32.cc#newcode4637
src/ia32/code-stubs-ia32.cc:4637: if (FLAG_debug_code) {
Thx for this debug code block, it's a good idea.
https://codereview.chromium.org/106453003/diff/60001/src/ic.h
File src/ic.h (right):
https://codereview.chromium.org/106453003/diff/60001/src/ic.h#newcode857
src/ic.h:857: bool CanCreateAllocationMementos() const {
Change Can to Could, because Should and Could are better matched than
Should and Can.
https://codereview.chromium.org/106453003/diff/60001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/106453003/diff/60001/src/objects-inl.h#newcode1364
src/objects-inl.h:1364: type <= FIRST_NONSTRING_TYPE;
Shouldn't it be < instead of <=? My impression is you want to allow
tracking of strings.
https://codereview.chromium.org/106453003/
--
--
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.
