Addressed all comments and fixed a *nasty* bug on x64.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):
https://chromiumcodereview.appspot.com/10382055/diff/1/src/flag-definitions.h#newcode200
src/flag-definitions.h:200: DEFINE_bool(array_index_dehoisting, true,
On 2012/05/08 13:46:26, Jakob wrote:
Is there a reason to keep the flag around?
I am removing the counters but temporarily keeping the flags.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h#newcode3992
src/hydrogen-instructions.h:3992: uint32_t index_offset_;
On 2012/05/08 13:46:26, Jakob wrote:
Why is this a uint32_t? In hydrogen.cc, you're passing in an int32_t
that indeed
can be negative.
This is unsigned because on ia32 and x64 we can only put positive
displacements in the machine code (I mean, embedded in the array element
access instruction).
Making this signed would generate more inconsistencies (or checks) in
Lithium.
That said, you are right, I can use an unsigned type in the signature of
DehoistArrayIndex (when I extract the value from the HConstant) and with
deal signed values only inside that function, and this will make the
code cleaner.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h#newcode3998
src/hydrogen-instructions.h:3998: HLoadKeyedFastDoubleElement(HValue*
elements, HValue* key) :
On 2012/05/08 13:46:26, Jakob wrote:
nit: the colon goes in the next line (see HLoadKeyedFastElement as an
example)
Done.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3122
src/hydrogen.cc:3122: // FIXME(mmassi): decide meaningful bound
On 2012/05/08 13:46:26, Jakob wrote:
fix this before landing? :-)
Done.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3127
src/hydrogen.cc:3127: counters->array_indexes_removed()->Increment();
On 2012/05/08 13:46:26, Jakob wrote:
Is it useful to keep those counters, or have they served their
purpose?
Done.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3143
src/hydrogen.cc:3143: if (instr->IsLoadKeyedFastElement()) {
On 2012/05/08 13:46:26, Jakob wrote:
class ArrayInstructionInterface {
[...]
Oddly enough, this was my original idea but Sven decided against it,
likely on the base that we would still need the switch to detect those 6
instructions.
But I still like the "interface" approach more so I will happily
implement it.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2433
src/ia32/lithium-codegen-ia32.cc:2433: instr->additional_index()));
On 2012/05/08 13:46:26, Jakob wrote:
You can use "instr->hydrogen()->index_offset()" here. (Same below.)
Done.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2458
src/ia32/lithium-codegen-ia32.cc:2458: instr->elements(),
On 2012/05/08 13:46:26, Jakob wrote:
Leaving these on one line is fine, actually. But if you prefer, you
can split
them.
Done.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2472
src/ia32/lithium-codegen-ia32.cc:2472: uint32_t additional_index) {
On 2012/05/08 13:46:26, Jakob wrote:
int32_t
See above.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2481
src/ia32/lithium-codegen-ia32.cc:2481: (constant_value +
additional_index) * (1 << shift_size)
On 2012/05/09 10:32:27, fschneider wrote:
You could just rewrite
expr * (1 << shift_size) into
expr << shift_size
Done (on all archs).
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode3485
src/ia32/lithium-codegen-ia32.cc:3485: - kHeapObjectTag,
On 2012/05/08 13:46:26, Jakob wrote:
Nit: this is ugly, please move all parameters to the next line,
indented by 4
spaces.
Done.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.h
File src/ia32/lithium-codegen-ia32.h (right):
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.h#newcode246
src/ia32/lithium-codegen-ia32.h:246: uint32_t additional_index = 0);
On 2012/05/08 13:46:26, Jakob wrote:
int32_t please.
uint32_t everywhere :-)
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.cc#newcode1964
src/ia32/lithium-ia32.cc:1964: instr->index_offset());
On 2012/05/08 13:46:26, Jakob wrote:
Again, I think you don't need any of the changes to this file.
Well, I decided to add an accessor just for convenience (like in the
other Lithium files)...
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):
https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.h#newcode1250
src/ia32/lithium-ia32.h:1250: uint32_t additional_index_;
On 2012/05/08 13:46:26, Jakob wrote:
You don't need to pass this value to the LInstruction. Just use the
hydrogen()
accessor to fetch it from the associated HInstruction.
I think that obsoletes all of the changes to this file.
Done.
https://chromiumcodereview.appspot.com/10382055/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
