Reviewers: Benedikt Meurer,
Description:
Check that environments assigned via AssignEnvironment are actually used.
Removed some temporary marker comments on the way.
Please review this at https://codereview.chromium.org/218403006/
SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge
Affected files (+58, -32 lines):
M src/arm/lithium-arm.cc
M src/arm/lithium-codegen-arm.cc
M src/arm64/lithium-arm64.cc
M src/arm64/lithium-codegen-arm64.cc
M src/ia32/lithium-codegen-ia32.cc
M src/ia32/lithium-ia32.cc
M src/lithium.h
M src/lithium.cc
M src/lithium-codegen.h
M src/lithium-codegen.cc
M src/x64/lithium-codegen-x64.cc
M src/x64/lithium-x64.cc
Index: src/arm/lithium-arm.cc
diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc
index
38acd48f3c7b89f3d6d3c819689531a745fc7097..97722ccfa24876d8671978cbabfc49f5ce8fab64
100644
--- a/src/arm/lithium-arm.cc
+++ b/src/arm/lithium-arm.cc
@@ -623,6 +623,8 @@ LInstruction* LChunkBuilder::MarkAsCall(LInstruction*
instr,
!hinstr->HasObservableSideEffects();
if (needs_environment && !instr->HasEnvironment()) {
instr = AssignEnvironment(instr);
+ // We can't really figure out if the environment is needed or not.
+ instr->environment()->set_has_been_used();
}
return instr;
@@ -1895,10 +1897,7 @@ LInstruction* LChunkBuilder::DoChange(HChange*
instr) {
LOperand* temp2 = FixedTemp(d11);
LInstruction* result =
DefineSameAsFirst(new(zone()) LTaggedToI(value, temp1, temp2));
- if (!val->representation().IsSmi()) {
- // Note: Only deopts in deferred code.
- result = AssignEnvironment(result);
- }
+ if (!val->representation().IsSmi()) result =
AssignEnvironment(result);
return result;
}
}
@@ -1993,11 +1992,10 @@ LInstruction*
LChunkBuilder::DoCheckMaps(HCheckMaps* instr) {
value = UseRegisterAtStart(instr->value());
if (instr->has_migration_target()) info()->MarkAsDeferredCalling();
}
- LCheckMaps* result = new(zone()) LCheckMaps(value);
+ LInstruction* result = new(zone()) LCheckMaps(value);
if (!instr->CanOmitMapChecks()) {
- // Note: Only deopts in deferred code.
- AssignEnvironment(result);
- if (instr->has_migration_target()) return AssignPointerMap(result);
+ result = AssignEnvironment(result);
+ if (instr->has_migration_target()) result = AssignPointerMap(result);
}
return result;
}
Index: src/arm/lithium-codegen-arm.cc
diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc
index
63e60ba66b00a11c106d4dee938fcdf0c316e07c..7b3788e71006efb8ba5ff9d2e80bb1c751d13d0a
100644
--- a/src/arm/lithium-codegen-arm.cc
+++ b/src/arm/lithium-codegen-arm.cc
@@ -783,6 +783,7 @@ void
LCodeGen::CallRuntimeFromDeferred(Runtime::FunctionId id,
void LCodeGen::RegisterEnvironmentForDeoptimization(LEnvironment*
environment,
Safepoint::DeoptMode
mode) {
+ environment->set_has_been_used();
if (!environment->HasBeenRegistered()) {
// Physical stack frame layout:
// -x ............. -4 0 ..................................... y
Index: src/arm64/lithium-arm64.cc
diff --git a/src/arm64/lithium-arm64.cc b/src/arm64/lithium-arm64.cc
index
1888affd683f74e78ebea1c6f441c0f7585c063d..d58e3cc9916bf01890c550f2b581ae158f17c71c
100644
--- a/src/arm64/lithium-arm64.cc
+++ b/src/arm64/lithium-arm64.cc
@@ -515,6 +515,8 @@ LInstruction* LChunkBuilder::MarkAsCall(LInstruction*
instr,
!hinstr->HasObservableSideEffects();
if (needs_environment && !instr->HasEnvironment()) {
instr = AssignEnvironment(instr);
+ // We can't really figure out if the environment is needed or not.
+ instr->environment()->set_has_been_used();
}
return instr;
@@ -1107,10 +1109,7 @@ LInstruction* LChunkBuilder::DoChange(HChange*
instr) {
LOperand* temp2 = instr->CanTruncateToInt32() ? NULL :
FixedTemp(d24);
LInstruction* result =
DefineAsRegister(new(zone()) LTaggedToI(value, temp1, temp2));
- if (!val->representation().IsSmi()) {
- // Note: Only deopts in deferred code.
- result = AssignEnvironment(result);
- }
+ if (!val->representation().IsSmi()) result =
AssignEnvironment(result);
return result;
}
}
@@ -1194,9 +1193,8 @@ LInstruction* LChunkBuilder::DoCheckMaps(HCheckMaps*
instr) {
}
LInstruction* result = new(zone()) LCheckMaps(value, temp);
if (!instr->CanOmitMapChecks()) {
- // Note: Only deopts in deferred code.
result = AssignEnvironment(result);
- if (instr->has_migration_target()) return AssignPointerMap(result);
+ if (instr->has_migration_target()) result = AssignPointerMap(result);
}
return result;
}
@@ -2432,7 +2430,6 @@ LInstruction*
LChunkBuilder::DoUnaryMathOperation(HUnaryMathOperation* instr) {
LOperand* temp3 = TempRegister();
LInstruction* result = DefineAsRegister(
new(zone()) LMathAbsTagged(context, input, temp1, temp2,
temp3));
- // Note: Only deopts in deferred code.
return AssignEnvironment(AssignPointerMap(result));
} else {
LOperand* input = UseRegisterAtStart(instr->value());
Index: src/arm64/lithium-codegen-arm64.cc
diff --git a/src/arm64/lithium-codegen-arm64.cc
b/src/arm64/lithium-codegen-arm64.cc
index
9b8156e754e05d6bfb8dbf13f4e2cd91fcdf34c2..48ea7796773e0b3cb4f5638b597abd967d29f56f
100644
--- a/src/arm64/lithium-codegen-arm64.cc
+++ b/src/arm64/lithium-codegen-arm64.cc
@@ -376,6 +376,7 @@ int
LCodeGen::DefineDeoptimizationLiteral(Handle<Object> literal) {
void LCodeGen::RegisterEnvironmentForDeoptimization(LEnvironment*
environment,
Safepoint::DeoptMode
mode) {
+ environment->set_has_been_used();
if (!environment->HasBeenRegistered()) {
int frame_count = 0;
int jsframe_count = 0;
Index: src/ia32/lithium-codegen-ia32.cc
diff --git a/src/ia32/lithium-codegen-ia32.cc
b/src/ia32/lithium-codegen-ia32.cc
index
6c934b7ce9a7654a3b8fd6faa4238a2481f899dd..47cb2dcbf06f1cbd2b716ff7b67cb00eefc008bc
100644
--- a/src/ia32/lithium-codegen-ia32.cc
+++ b/src/ia32/lithium-codegen-ia32.cc
@@ -1051,6 +1051,7 @@ void
LCodeGen::CallRuntimeFromDeferred(Runtime::FunctionId id,
void LCodeGen::RegisterEnvironmentForDeoptimization(
LEnvironment* environment, Safepoint::DeoptMode mode) {
+ environment->set_has_been_used();
if (!environment->HasBeenRegistered()) {
// Physical stack frame layout:
// -x ............. -4 0 ..................................... y
Index: src/ia32/lithium-ia32.cc
diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc
index
c230deba2882abd2e16e65b9d5a76c19b5594628..66c79d1d587c2cf1857789728993c87e2cc97f43
100644
--- a/src/ia32/lithium-ia32.cc
+++ b/src/ia32/lithium-ia32.cc
@@ -688,6 +688,8 @@ LInstruction* LChunkBuilder::MarkAsCall(LInstruction*
instr,
!hinstr->HasObservableSideEffects();
if (needs_environment && !instr->HasEnvironment()) {
instr = AssignEnvironment(instr);
+ // We can't really figure out if the environment is needed or not.
+ instr->environment()->set_has_been_used();
}
return instr;
@@ -1934,10 +1936,7 @@ LInstruction* LChunkBuilder::DoChange(HChange*
instr) {
? FixedTemp(xmm1) : NULL;
LInstruction* result =
DefineSameAsFirst(new(zone()) LTaggedToI(value, xmm_temp));
- if (!val->representation().IsSmi()) {
- // Note: Only deopts in deferred code.
- result = AssignEnvironment(result);
- }
+ if (!val->representation().IsSmi()) result =
AssignEnvironment(result);
return result;
}
}
@@ -2043,11 +2042,10 @@ LInstruction*
LChunkBuilder::DoCheckMaps(HCheckMaps* instr) {
value = UseRegisterAtStart(instr->value());
if (instr->has_migration_target()) info()->MarkAsDeferredCalling();
}
- LCheckMaps* result = new(zone()) LCheckMaps(value);
+ LInstruction* result = new(zone()) LCheckMaps(value);
if (!instr->CanOmitMapChecks()) {
- // Note: Only deopts in deferred code.
- AssignEnvironment(result);
- if (instr->has_migration_target()) return AssignPointerMap(result);
+ result = AssignEnvironment(result);
+ if (instr->has_migration_target()) result = AssignPointerMap(result);
}
return result;
}
Index: src/lithium-codegen.cc
diff --git a/src/lithium-codegen.cc b/src/lithium-codegen.cc
index
be0ff8371afa51ec664d61a3056d2da13ae7ff79..93ae3101a7779e50b21fe4e542334974bc75758e
100644
--- a/src/lithium-codegen.cc
+++ b/src/lithium-codegen.cc
@@ -122,6 +122,26 @@ bool LCodeGenBase::GenerateBody() {
}
+void LCodeGenBase::CheckEnvironmentUsage() {
+#ifdef DEBUG
+ bool live_block = true;
+ for (int i = 0; i < instructions_->length(); i++) {
+ LInstruction* instr = instructions_->at(i);
+ if (instr->IsLabel()) live_block
= !LLabel::cast(instr)->HasReplacement();
+ if (live_block &&
+ instr->hydrogen_value()->block()->IsReachable() &&
+ instr->HasEnvironment() &&
+ !instr->environment()->has_been_used()) {
+ FunctionLiteral* lit = info_->function();
+ V8_Fatal(__FILE__, __LINE__, "unused environment in %s
<@%d,#%d> %s\n",
+ lit == NULL ? "<UNKNOWN>" : lit->name()->ToCString().get(),
+ i, instr->hydrogen_value()->id(), instr->Mnemonic());
+ }
+ }
+#endif
+}
+
+
void LCodeGenBase::Comment(const char* format, ...) {
if (!FLAG_code_comments) return;
char buffer[4 * KB];
Index: src/lithium-codegen.h
diff --git a/src/lithium-codegen.h b/src/lithium-codegen.h
index
3e8d471ea78deb92f7f5647dd59cebd116f5ffd6..9f06781b852a0800b72602489a475bb2333eac58
100644
--- a/src/lithium-codegen.h
+++ b/src/lithium-codegen.h
@@ -68,6 +68,11 @@ class LCodeGenBase BASE_EMBEDDED {
void RegisterWeakObjectsInOptimizedCode(Handle<Code> code);
+ // Check that an environment assigned via AssignEnvironment is actually
being
+ // used. Redundant assignments keep things alive longer than necessary,
and
+ // consequently lead to worse code, so it's important to minimize this.
+ void CheckEnvironmentUsage();
+
protected:
enum Status {
UNUSED,
Index: src/lithium.cc
diff --git a/src/lithium.cc b/src/lithium.cc
index
8753ff14aa4f0b0cfbd1fe4b4cd6b2b151da2a20..48567930abc1c3bec19418e4e974a7fc335497d5
100644
--- a/src/lithium.cc
+++ b/src/lithium.cc
@@ -432,6 +432,7 @@ Handle<Code> LChunk::Codegen() {
MarkEmptyBlocks();
if (generator.GenerateCode()) {
+ generator.CheckEnvironmentUsage();
CodeGenerator::MakeCodePrologue(info(), "optimized");
Code::Flags flags = info()->flags();
Handle<Code> code =
Index: src/lithium.h
diff --git a/src/lithium.h b/src/lithium.h
index
8ae5b879dc7bbe6b6f53388b8efec5011d1a89d0..161da8f0a3b6e8ecc0ee8f3cf36add6103db63a1
100644
--- a/src/lithium.h
+++ b/src/lithium.h
@@ -426,7 +426,8 @@ class LEnvironment V8_FINAL : public ZoneObject {
object_mapping_(0, zone),
outer_(outer),
entry_(entry),
- zone_(zone) { }
+ zone_(zone),
+ has_been_used_(false) { }
Handle<JSFunction> closure() const { return closure_; }
FrameType frame_type() const { return frame_type_; }
@@ -442,6 +443,9 @@ class LEnvironment V8_FINAL : public ZoneObject {
HEnterInlined* entry() { return entry_; }
Zone* zone() const { return zone_; }
+ bool has_been_used() const { return has_been_used_; }
+ void set_has_been_used() { has_been_used_ = true; }
+
void AddValue(LOperand* operand,
Representation representation,
bool is_uint32) {
@@ -541,6 +545,7 @@ class LEnvironment V8_FINAL : public ZoneObject {
LEnvironment* outer_;
HEnterInlined* entry_;
Zone* zone_;
+ bool has_been_used_;
};
Index: src/x64/lithium-codegen-x64.cc
diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc
index
a64d3e223fae20d9f17dbb74b1580d58ed847ed3..a8536a1841f379a5078e5fc79716f16e9564024f
100644
--- a/src/x64/lithium-codegen-x64.cc
+++ b/src/x64/lithium-codegen-x64.cc
@@ -687,6 +687,7 @@ void
LCodeGen::CallRuntimeFromDeferred(Runtime::FunctionId id,
void LCodeGen::RegisterEnvironmentForDeoptimization(LEnvironment*
environment,
Safepoint::DeoptMode
mode) {
+ environment->set_has_been_used();
if (!environment->HasBeenRegistered()) {
// Physical stack frame layout:
// -x ............. -4 0 ..................................... y
Index: src/x64/lithium-x64.cc
diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc
index
d1c77311ecf9da440af7748ffbf7567f6e5c78c2..90f9a1a236f72eb1b23dac9fcea4262acb986b43
100644
--- a/src/x64/lithium-x64.cc
+++ b/src/x64/lithium-x64.cc
@@ -652,6 +652,8 @@ LInstruction* LChunkBuilder::MarkAsCall(LInstruction*
instr,
!hinstr->HasObservableSideEffects();
if (needs_environment && !instr->HasEnvironment()) {
instr = AssignEnvironment(instr);
+ // We can't really figure out if the environment is needed or not.
+ instr->environment()->set_has_been_used();
}
return instr;
@@ -1857,10 +1859,7 @@ LInstruction* LChunkBuilder::DoChange(HChange*
instr) {
LOperand* xmm_temp = truncating ? NULL : FixedTemp(xmm1);
LInstruction* result =
DefineSameAsFirst(new(zone()) LTaggedToI(value, xmm_temp));
- if (!val->representation().IsSmi()) {
- // Note: Only deopts in deferred code.
- result = AssignEnvironment(result);
- }
+ if (!val->representation().IsSmi()) result =
AssignEnvironment(result);
return result;
}
}
@@ -1955,11 +1954,10 @@ LInstruction*
LChunkBuilder::DoCheckMaps(HCheckMaps* instr) {
value = UseRegisterAtStart(instr->value());
if (instr->has_migration_target()) info()->MarkAsDeferredCalling();
}
- LCheckMaps* result = new(zone()) LCheckMaps(value);
+ LInstruction* result = new(zone()) LCheckMaps(value);
if (!instr->CanOmitMapChecks()) {
- // Note: Only deopts in deferred code.
- AssignEnvironment(result);
- if (instr->has_migration_target()) return AssignPointerMap(result);
+ result = AssignEnvironment(result);
+ if (instr->has_migration_target()) result = AssignPointerMap(result);
}
return result;
}
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
