From 11311c083abb37762ea979a25bf1f21c11928b20 Mon Sep 17 00:00:00 2001 From: titzer Date: Thu, 29 Jan 2015 09:40:10 -0800 Subject: [PATCH] [turbofan] Fix OSR compilations of for-in. R=mstarzinger@chromium.org LOG=Y BUG= Review URL: https://codereview.chromium.org/890543002 Cr-Commit-Position: refs/heads/master@{#26333} --- src/compiler.cc | 16 +++- src/compiler.h | 4 + src/compiler/ast-graph-builder.cc | 172 +++++++++++++++++++------------------ src/compiler/ast-graph-builder.h | 1 + src/compiler/graph-builder.cc | 15 ++-- test/mjsunit/asm/int32modb.js | 26 ++++++ test/mjsunit/compiler/osr-forin.js | 26 ++++++ test/mjsunit/compiler/osr-forof.js | 35 ++++++++ 8 files changed, 199 insertions(+), 96 deletions(-) create mode 100644 test/mjsunit/asm/int32modb.js create mode 100644 test/mjsunit/compiler/osr-forin.js create mode 100644 test/mjsunit/compiler/osr-forof.js diff --git a/src/compiler.cc b/src/compiler.cc index 978d571..7766778 100644 --- a/src/compiler.cc +++ b/src/compiler.cc @@ -23,6 +23,7 @@ #include "src/liveedit.h" #include "src/messages.h" #include "src/parser.h" +#include "src/prettyprinter.h" #include "src/rewriter.h" #include "src/runtime-profiler.h" #include "src/scanner-character-streams.h" @@ -412,7 +413,9 @@ OptimizedCompileJob::Status OptimizedCompileJob::CreateGraph() { if (FLAG_trace_opt) { OFStream os(stdout); os << "[compiling method " << Brief(*info()->closure()) - << " using TurboFan]" << std::endl; + << " using TurboFan"; + if (info()->is_osr()) os << " OSR"; + os << "]" << std::endl; } Timer t(this, &time_taken_to_create_graph_); compiler::Pipeline pipeline(info()); @@ -425,7 +428,9 @@ OptimizedCompileJob::Status OptimizedCompileJob::CreateGraph() { if (FLAG_trace_opt) { OFStream os(stdout); os << "[compiling method " << Brief(*info()->closure()) - << " using Crankshaft]" << std::endl; + << " using Crankshaft"; + if (info()->is_osr()) os << " OSR"; + os << "]" << std::endl; } if (FLAG_trace_hydrogen) { @@ -1589,4 +1594,11 @@ bool CompilationPhase::ShouldProduceTraceOutput() const { base::OS::StrChr(const_cast(FLAG_trace_phase), name_[0]) != NULL); } + +#if DEBUG +void CompilationInfo::PrintAstForTesting() { + PrintF("--- Source from AST ---\n%s\n", + PrettyPrinter(isolate(), zone()).PrintProgram(function())); +} +#endif } } // namespace v8::internal diff --git a/src/compiler.h b/src/compiler.h index 9901fe6..11cec55 100644 --- a/src/compiler.h +++ b/src/compiler.h @@ -405,6 +405,10 @@ class CompilationInfo { ast_value_factory_owned_ = owned; } +#if DEBUG + void PrintAstForTesting(); +#endif + protected: CompilationInfo(Handle shared_info, Zone* zone); diff --git a/src/compiler/ast-graph-builder.cc b/src/compiler/ast-graph-builder.cc index 7b62443..ce9accc 100644 --- a/src/compiler/ast-graph-builder.cc +++ b/src/compiler/ast-graph-builder.cc @@ -702,90 +702,9 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { environment()->Push(cache_array); environment()->Push(cache_length); environment()->Push(jsgraph()->ZeroConstant()); - // PrepareForBailoutForId(stmt->BodyId(), NO_REGISTERS); - LoopBuilder for_loop(this); - for_loop.BeginLoop(GetVariablesAssignedInLoop(stmt), - IsOsrLoopEntry(stmt)); - // Check loop termination condition. - Node* index = environment()->Peek(0); - Node* exit_cond = - NewNode(javascript()->LessThan(), index, cache_length); - // TODO(jarin): provide real bailout id. - PrepareFrameState(exit_cond, BailoutId::None()); - for_loop.BreakUnless(exit_cond); - // TODO(dcarney): this runtime call should be a handful of - // simplified instructions that - // basically produce - // value = array[index] - environment()->Push(obj); - environment()->Push(cache_array); - environment()->Push(cache_type); - environment()->Push(index); - Node* pair = ProcessArguments( - javascript()->CallRuntime(Runtime::kForInNext, 4), 4); - Node* value = NewNode(common()->Projection(0), pair); - Node* should_filter = NewNode(common()->Projection(1), pair); - environment()->Push(value); - { - // Test if FILTER_KEY needs to be called. - IfBuilder test_should_filter(this); - Node* should_filter_cond = - NewNode(javascript()->StrictEqual(), should_filter, - jsgraph()->TrueConstant()); - test_should_filter.If(should_filter_cond); - test_should_filter.Then(); - value = environment()->Pop(); - Node* builtins = BuildLoadBuiltinsObject(); - Node* function = BuildLoadObjectField( - builtins, - JSBuiltinsObject::OffsetOfFunctionWithId(Builtins::FILTER_KEY)); - // Callee. - environment()->Push(function); - // Receiver. - environment()->Push(obj); - // Args. - environment()->Push(value); - // result is either the string key or Smi(0) indicating the property - // is gone. - Node* res = ProcessArguments( - javascript()->CallFunction(3, NO_CALL_FUNCTION_FLAGS), 3); - // TODO(jarin): provide real bailout id. - PrepareFrameState(res, BailoutId::None()); - Node* property_missing = NewNode(javascript()->StrictEqual(), res, - jsgraph()->ZeroConstant()); - { - IfBuilder is_property_missing(this); - is_property_missing.If(property_missing); - is_property_missing.Then(); - // Inc counter and continue. - Node* index_inc = - NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); - // TODO(jarin): provide real bailout id. - PrepareFrameState(index_inc, BailoutId::None()); - environment()->Poke(0, index_inc); - for_loop.Continue(); - is_property_missing.Else(); - is_property_missing.End(); - } - // Replace 'value' in environment. - environment()->Push(res); - test_should_filter.Else(); - test_should_filter.End(); - } - value = environment()->Pop(); - // Bind value and do loop body. - VisitForInAssignment(stmt->each(), value, stmt->AssignmentId()); - VisitIterationBody(stmt, &for_loop, 5); - for_loop.EndBody(); - // Inc counter and continue. - Node* index_inc = - NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); - // TODO(jarin): provide real bailout id. - PrepareFrameState(index_inc, BailoutId::None()); - environment()->Poke(0, index_inc); - for_loop.EndLoop(); - environment()->Drop(5); - // PrepareForBailoutForId(stmt->ExitId(), NO_REGISTERS); + + // Build the actual loop body. + VisitForInBody(stmt); } have_no_properties.End(); } @@ -795,6 +714,91 @@ void AstGraphBuilder::VisitForInStatement(ForInStatement* stmt) { } +// TODO(dcarney): this is a big function. Try to clean up some. +void AstGraphBuilder::VisitForInBody(ForInStatement* stmt) { + LoopBuilder for_loop(this); + for_loop.BeginLoop(GetVariablesAssignedInLoop(stmt), IsOsrLoopEntry(stmt)); + + // These stack values are renamed in the case of OSR, so reload them + // from the environment. + Node* index = environment()->Peek(0); + Node* cache_length = environment()->Peek(1); + Node* cache_array = environment()->Peek(2); + Node* cache_type = environment()->Peek(3); + Node* obj = environment()->Peek(4); + + // Check loop termination condition. + Node* exit_cond = NewNode(javascript()->LessThan(), index, cache_length); + // TODO(jarin): provide real bailout id. + PrepareFrameState(exit_cond, BailoutId::None()); + for_loop.BreakUnless(exit_cond); + Node* pair = NewNode(javascript()->CallRuntime(Runtime::kForInNext, 4), obj, + cache_array, cache_type, index); + Node* value = NewNode(common()->Projection(0), pair); + Node* should_filter = NewNode(common()->Projection(1), pair); + environment()->Push(value); + { + // Test if FILTER_KEY needs to be called. + IfBuilder test_should_filter(this); + Node* should_filter_cond = NewNode( + javascript()->StrictEqual(), should_filter, jsgraph()->TrueConstant()); + test_should_filter.If(should_filter_cond); + test_should_filter.Then(); + value = environment()->Pop(); + Node* builtins = BuildLoadBuiltinsObject(); + Node* function = BuildLoadObjectField( + builtins, + JSBuiltinsObject::OffsetOfFunctionWithId(Builtins::FILTER_KEY)); + // Callee. + environment()->Push(function); + // Receiver. + environment()->Push(obj); + // Args. + environment()->Push(value); + // result is either the string key or Smi(0) indicating the property + // is gone. + Node* res = ProcessArguments( + javascript()->CallFunction(3, NO_CALL_FUNCTION_FLAGS), 3); + // TODO(jarin): provide real bailout id. + PrepareFrameState(res, BailoutId::None()); + Node* property_missing = + NewNode(javascript()->StrictEqual(), res, jsgraph()->ZeroConstant()); + { + IfBuilder is_property_missing(this); + is_property_missing.If(property_missing); + is_property_missing.Then(); + // Inc counter and continue. + Node* index_inc = + NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); + // TODO(jarin): provide real bailout id. + PrepareFrameState(index_inc, BailoutId::None()); + environment()->Poke(0, index_inc); + for_loop.Continue(); + is_property_missing.Else(); + is_property_missing.End(); + } + // Replace 'value' in environment. + environment()->Push(res); + test_should_filter.Else(); + test_should_filter.End(); + } + value = environment()->Pop(); + // Bind value and do loop body. + VisitForInAssignment(stmt->each(), value, stmt->AssignmentId()); + VisitIterationBody(stmt, &for_loop, 5); + for_loop.EndBody(); + // Inc counter and continue. + Node* index_inc = + NewNode(javascript()->Add(), index, jsgraph()->OneConstant()); + // TODO(jarin): provide real bailout id. + PrepareFrameState(index_inc, BailoutId::None()); + environment()->Poke(0, index_inc); + for_loop.EndLoop(); + environment()->Drop(5); + // PrepareForBailoutForId(stmt->ExitId(), NO_REGISTERS); +} + + void AstGraphBuilder::VisitForOfStatement(ForOfStatement* stmt) { LoopBuilder for_loop(this); VisitForEffect(stmt->assign_iterator()); diff --git a/src/compiler/ast-graph-builder.h b/src/compiler/ast-graph-builder.h index ce13374..175228d 100644 --- a/src/compiler/ast-graph-builder.h +++ b/src/compiler/ast-graph-builder.h @@ -199,6 +199,7 @@ class AstGraphBuilder : public StructuredGraphBuilder, public AstVisitor { // Dispatched from VisitForInStatement. void VisitForInAssignment(Expression* expr, Node* value, BailoutId bailout_id); + void VisitForInBody(ForInStatement* stmt); // Dispatched from VisitClassLiteral. void VisitClassLiteralContents(ClassLiteral* expr); diff --git a/src/compiler/graph-builder.cc b/src/compiler/graph-builder.cc index 181af2f..aa268c0 100644 --- a/src/compiler/graph-builder.cc +++ b/src/compiler/graph-builder.cc @@ -201,16 +201,11 @@ void StructuredGraphBuilder::Environment::PrepareForLoop(BitVector* assigned, for (int i = 0; i < size; ++i) { Node* val = values()->at(i); - // TODO(titzer): use IrOpcode::IsConstant() or similar. - if (val->opcode() == IrOpcode::kNumberConstant || - val->opcode() == IrOpcode::kInt32Constant || - val->opcode() == IrOpcode::kInt64Constant || - val->opcode() == IrOpcode::kFloat64Constant || - val->opcode() == IrOpcode::kHeapConstant) - continue; - Node* osr_value = - graph->NewNode(builder_->common()->OsrValue(i), osr_loop_entry); - values()->at(i) = builder_->MergeValue(val, osr_value, control); + if (!IrOpcode::IsConstantOpcode(val->opcode())) { + Node* osr_value = + graph->NewNode(builder_->common()->OsrValue(i), osr_loop_entry); + values()->at(i) = builder_->MergeValue(val, osr_value, control); + } } } } diff --git a/test/mjsunit/asm/int32modb.js b/test/mjsunit/asm/int32modb.js new file mode 100644 index 0000000..5081b49 --- /dev/null +++ b/test/mjsunit/asm/int32modb.js @@ -0,0 +1,26 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +var stdlib = {}; +var foreign = {}; +var heap = new ArrayBuffer(64 * 1024); + +var mod = (function Module(stdlib, foreign, heap) { + "use asm"; + function mod(dividend, divisor) { + dividend = dividend|0; + divisor = divisor|0; + return (dividend % divisor) | 0; + } + return { mod: mod }; +})(stdlib, foreign, heap).mod; + +var divisors = [-2147483648, -32 * 1024, -1000, -16, -7, -2, -1, 0, + 1, 3, 4, 10, 64, 99, 1023, 1024, 2147483647]; +for (var i = 0; i < divisors.length; i++) { + var divisor = divisors[i]; + for (var dividend = -2147483648; dividend < 2147483648; dividend += 3999773) { + assertEquals((dividend % divisor) | 0, mod(dividend, divisor)); + } +} diff --git a/test/mjsunit/compiler/osr-forin.js b/test/mjsunit/compiler/osr-forin.js new file mode 100644 index 0000000..37e5f22 --- /dev/null +++ b/test/mjsunit/compiler/osr-forin.js @@ -0,0 +1,26 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax --use-osr --turbo-osr + +function f(a) { + var sum = 0; + for (var j in a) { + var i = a[j]; + var x = i + 2; + var y = x + 5; + var z = y + 3; + sum += z; + } + return sum; +} + +var a = new Array(); +for (var i = 0; i < 10000; i++) { + a[i] = (i * 999) % 77; +} + +for (var i = 0; i < 3; i++) { + assertEquals(480270, f(a)); +} diff --git a/test/mjsunit/compiler/osr-forof.js b/test/mjsunit/compiler/osr-forof.js new file mode 100644 index 0000000..87ae652 --- /dev/null +++ b/test/mjsunit/compiler/osr-forof.js @@ -0,0 +1,35 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax --use-osr --turbo-osr + +function f(a) { + var sum = 0; + for (var i of a) { + var x = i + 2; + var y = x + 5; + var z = y + 3; + sum += z; + } + return sum; +} + +var a = new Array(); +for (var i = 0; i < 10000; i++) { + a[i] = (i * 999) % 77; +} + +for (var i = 0; i < 3; i++) { + assertEquals(480270, f(wrap(a))); +} + +function wrap(array) { + var iterable = {}; + var i = 0; + function next() { + return { done: i >= array.length, value: array[i++] }; + }; + iterable[Symbol.iterator] = function() { return { next:next }; }; + return iterable; +} -- 2.7.4