Looks almost good! Can you also fix the redundant index-additions that may
arrive in the example we discussed offline? I think it will improve the
benefits
of your optimization noticable.
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2884
src/hydrogen.cc:2884: AssertNoAllocation no_gc;
On 2012/04/25 12:19:19, Massi wrote:
On 2012/04/25 09:40:48, fschneider wrote:
> Why would you want to assert no allocation here? It does not hurt,
but does
not
> seem necessary.
It is necessary: I was hitting asserts without it.
This happened computing key hashes calling HConstant::Hashcode().
I don't believe it is necessary. If anything it will trigger an assert
once you allocate a heap object on the JS heap in your code.
I don't this is currently the case, and even if it would, we can deal
with allocations inside the compiler since everything is handlified.
http://codereview.chromium.org/10032029/diff/33002/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/10032029/diff/33002/src/hydrogen.cc#newcode2687
src/hydrogen.cc:2687: int32_t new_offset) {
Fits in previous line?
http://codereview.chromium.org/10032029/diff/33002/src/hydrogen.cc#newcode2775
src/hydrogen.cc:2775: (*add)->DeleteAndReplaceWith((*add)->left());
Could you add a test case that exercises this code inside the
if-statement? When running your added it, I don't find this path to be
executed.
http://codereview.chromium.org/10032029/diff/33002/test/mjsunit/array-bounds-check-removal.js
File test/mjsunit/array-bounds-check-removal.js (right):
http://codereview.chromium.org/10032029/diff/33002/test/mjsunit/array-bounds-check-removal.js#newcode144
test/mjsunit/array-bounds-check-removal.js:144: gc();
Why do you need --expose-gc and a call gc() here?
http://codereview.chromium.org/10032029/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
