Hi Danno,
Thanks a lot~
I constructed some test cases to check various usage scenarios. Yes, you
concern
is correct. Some special HValue does not generate code explicitly, like
HConstant, which may be emitted as Uses. In below case, -3 need to sign
extend
to 64 bit when moving to a register instead of int32. I fix this issue when
resolving the Gap.
function foo() {
var ret = 0;
for (var i = -3; i < 4; i += 1) {
if (i < 0) {
ret += a[i + 4];
} else {
ret += a[i];
}
}
return ret;
}
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-codegen-x64.cc#newcode278
src/x64/lithium-codegen-x64.cc:278: Register result_reg =
ToRegister(instr->result());
On 2014/03/18 08:21:31, danno wrote:
I think this might be a problem for some HValues that don't guarantee
their
result is in a register, e.g. HParameters.
The testcase shows a HChange between HParameters and Load/StoreKeyed to
convert the tagged value to an integer. Sign extension work on the
result of HChange
Done.
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-x64.cc#newcode1979
src/x64/lithium-x64.cc:1979: void RecordKeyValueThroughPhi(HValue* phi,
BitVector* key_values) {
On 2014/03/18 08:20:20, danno wrote:
This should be called FindDehoistedKeyDefinitions, make it a private
member of
the ChunkBuilder so that you don't have to pass in the BitVector.
void LChunkBuilder::FindDehoistedKeyDefinitions(HValue* camdidate) {
if (dehoisted_key_ids_->Contains(camdidate->id())) return;
dehoisted_key_ids_->Add(value->id());
if (!candidate->IsPhi()) return;
for (int i = 0; i < candidate->OperandCount(); ++i) {
FindDehoistedKeyDefinitions(candidate->OperandAt(i));
}
}
The BitVector belongs to LChunk instead of LChunkBuilder. After making
this function private, BitVector is accessed through
chunk_->GetDehoistedKeyIds()
Done.
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-x64.cc#newcode2000
src/x64/lithium-x64.cc:2000: if
(!key_values->Contains(instr->key()->id())) {
On 2014/03/18 08:20:20, danno wrote:
Move most of the logic here into RecordKeyValueThroughPhi. The code
here should
be very simple:
if (instr->IsDehoisted()) {
FindDehoistedKeyDefinitions(instr->key());
}
Done.
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-x64.cc#newcode2046
src/x64/lithium-x64.cc:2046: BitVector* key_values =
chunk()->GetKeyValues();
On 2014/03/18 08:20:20, danno wrote:
See comment above.
Done.
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-x64.h
File src/x64/lithium-x64.h (right):
https://codereview.chromium.org/179773002/diff/70001/src/x64/lithium-x64.h#newcode2521
src/x64/lithium-x64.h:2521: BitVector key_values_;
On 2014/03/18 08:20:20, danno wrote:
This variable, the accesses for it and naming based on it should be
something
more like "dehoisted_key_ids_"
Done.
https://codereview.chromium.org/179773002/
--
--
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/d/optout.
