From b8990ff6ff967e3303a6b87177d424780646daae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Thu, 9 Jun 2022 08:31:22 +0000 Subject: [PATCH 1/8] cleaning up --- rir/src/compiler/analysis/context_stack.h | 2 +- rir/src/compiler/backend.cpp | 2 +- rir/src/compiler/opt/hoist_instruction.cpp | 3 +- rir/src/compiler/opt/inline.cpp | 847 ++++++++++----------- rir/src/compiler/opt/scope_resolution.cpp | 7 +- rir/src/compiler/pir/builder.cpp | 3 +- rir/src/compiler/pir/instruction.h | 17 +- rir/src/compiler/pir/singleton_values.h | 1 + rir/src/compiler/pir/tag.h | 7 +- rir/src/compiler/rir2pir/rir2pir.cpp | 2 +- rir/src/compiler/util/bb_transform.cpp | 4 +- rir/src/compiler/util/bb_transform.h | 6 +- 12 files changed, 448 insertions(+), 453 deletions(-) diff --git a/rir/src/compiler/analysis/context_stack.h b/rir/src/compiler/analysis/context_stack.h index 57964cd73..7f446165d 100644 --- a/rir/src/compiler/analysis/context_stack.h +++ b/rir/src/compiler/analysis/context_stack.h @@ -15,7 +15,7 @@ struct ContextStackState { if (auto mk = MkEnv::Cast((*i)->env())) it(mk); } - size_t context() const { return contextStack.size(); } + size_t numContexts() const { return contextStack.size(); } AbstractResult merge(const ContextStackState& other) { assert(contextStack.size() == other.contextStack.size() && "stack imbalance"); diff --git a/rir/src/compiler/backend.cpp b/rir/src/compiler/backend.cpp index 254801ee8..4da6f2976 100644 --- a/rir/src/compiler/backend.cpp +++ b/rir/src/compiler/backend.cpp @@ -58,7 +58,7 @@ static void approximateNeedsLdVarForUpdate( } } if (auto mk = MkEnv::Cast(ld->env())) - if (mk->stub && mk->arg(mk->indexOf(ld->varName)).val() != + if (mk->stub && mk->argNamed(ld->varName).val() != UnboundValue::instance()) return; diff --git a/rir/src/compiler/opt/hoist_instruction.cpp b/rir/src/compiler/opt/hoist_instruction.cpp index d0830d7b0..5b466e3f3 100644 --- a/rir/src/compiler/opt/hoist_instruction.cpp +++ b/rir/src/compiler/opt/hoist_instruction.cpp @@ -145,7 +145,8 @@ bool HoistInstruction::apply(Compiler& cmp, ClosureVersion* cls, Code* code, while (b->isEmpty()) b = *b->predecessors().begin(); - if (cs.after(b->last()).context() > cs.before(i).context()) { + if (cs.after(b->last()).numContexts() > + cs.before(i).numContexts()) { ip = next; continue; } diff --git a/rir/src/compiler/opt/inline.cpp b/rir/src/compiler/opt/inline.cpp index 525370872..33769d39b 100644 --- a/rir/src/compiler/opt/inline.cpp +++ b/rir/src/compiler/opt/inline.cpp @@ -36,486 +36,477 @@ bool Inline::apply(Compiler& cmp, ClosureVersion* cls, Code* code, return cls->rirFunction()->flags.contains(rir::Function::NotInlineable); }; - Visitor::run( - code->entry, [&](BB* bb) { - // Dangerous iterater usage, works since we do only update it in - // one place. - for (auto it = bb->begin(); it != bb->end() && fuel; it++) { - if (!CallInstruction::CastCall(*it)) + Visitor::run(code->entry, [&](BB* bb) { + // Dangerous iterater usage, works since we do only update it in + // one place. + for (auto it = bb->begin(); it != bb->end() && fuel; it++) { + if (!CallInstruction::CastCall(*it)) + continue; + + Closure* inlineeCls = nullptr; + ClosureVersion* inlinee = nullptr; + Value* staticEnv = nullptr; + + bool hasDotslistArg = false; + const FrameState* callerFrameState = nullptr; + if (auto call = Call::Cast(*it)) { + auto mk = MkCls::Cast(call->cls()->followCastsAndForce()); + if (!mk) continue; - - Closure* inlineeCls = nullptr; - ClosureVersion* inlinee = nullptr; - Value* staticEnv = nullptr; - - bool hasDotslistArg = false; - const FrameState* callerFrameState = nullptr; - if (auto call = Call::Cast(*it)) { - auto mk = MkCls::Cast(call->cls()->followCastsAndForce()); - if (!mk) - continue; - inlineeCls = mk->tryGetCls(); - if (!inlineeCls) - continue; - if (dontInline(inlineeCls)) - continue; - inlinee = call->tryDispatch(inlineeCls); - if (!inlinee) - continue; - bool hasDotArgs = false; - call->eachCallArg([&](Value* v) { - if (ExpandDots::Cast(v)) - hasDotArgs = true; - }); - // TODO do some argument matching - if (hasDotArgs) - continue; - staticEnv = mk->lexicalEnv(); - callerFrameState = call->frameState(); - } else if (auto call = StaticCall::Cast(*it)) { - inlineeCls = call->cls(); - if (dontInline(inlineeCls)) - continue; - inlinee = call->tryDispatch(); - if (!inlinee) - continue; - // if we don't know the closure of the inlinee, we can't - // inline. - staticEnv = inlineeCls->closureEnv(); - if (inlineeCls->closureEnv() == Env::notClosed() && - inlinee != cls) { - if (Query::noParentEnv(inlinee)) { - } else if (auto mk = - MkCls::Cast(call->runtimeClosure())) { - staticEnv = mk->lexicalEnv(); - } else if (call->runtimeClosure() != - Tombstone::closure()) { - static SEXP b = nullptr; - if (!b) { - auto idx = rir::blt("environment"); - b = Rf_allocSExp(BUILTINSXP); - b->u.primsxp.offset = idx; - R_PreserveObject(b); - } - auto e = new CallSafeBuiltin( - b, {call->runtimeClosure()}, 0); - e->type = PirType::env(); - e->effects.reset(); - it = bb->insert(it, e); - it++; - staticEnv = e; - } else { - continue; - } - } - call->eachCallArg([&](Value* v) { - assert(!ExpandDots::Cast(v)); - if (DotsList::Cast(v)) - hasDotslistArg = true; - }); - callerFrameState = call->frameState(); - } else { + inlineeCls = mk->tryGetCls(); + if (!inlineeCls) continue; - } - if (dontInline(inlineeCls)) continue; + inlinee = call->tryDispatch(inlineeCls); + if (!inlinee) + continue; + bool hasDotArgs = false; + call->eachCallArg([&](Value* v) { + if (ExpandDots::Cast(v)) + hasDotArgs = true; + }); + // TODO do some argument matching + if (hasDotArgs) + continue; + staticEnv = mk->lexicalEnv(); + callerFrameState = call->frameState(); + } else if (auto call = StaticCall::Cast(*it)) { + inlineeCls = call->cls(); + if (dontInline(inlineeCls)) + continue; + inlinee = call->tryDispatch(); + if (!inlinee) + continue; + // if we don't know the closure of the inlinee, we can't + // inline. + staticEnv = inlineeCls->closureEnv(); + if (inlineeCls->closureEnv() == Env::notClosed() && + inlinee != cls) { + if (Query::noParentEnv(inlinee)) { + } else if (auto mk = MkCls::Cast(call->runtimeClosure())) { + staticEnv = mk->lexicalEnv(); + } else if (call->runtimeClosure() != Tombstone::closure()) { + static SEXP b = nullptr; + if (!b) { + auto idx = rir::blt("environment"); + b = Rf_allocSExp(BUILTINSXP); + b->u.primsxp.offset = idx; + R_PreserveObject(b); + } + auto e = + new CallSafeBuiltin(b, {call->runtimeClosure()}, 0); + e->type = PirType::env(); + e->effects.reset(); + it = bb->insert(it, e); + it++; + staticEnv = e; + } else { + continue; + } + } + call->eachCallArg([&](Value* v) { + assert(!ExpandDots::Cast(v)); + if (DotsList::Cast(v)) + hasDotslistArg = true; + }); + callerFrameState = call->frameState(); + } else { + continue; + } - enum SafeToInline { - Yes, - NeedsContext, - No, - }; - - // TODO: instead of blacklisting those, we could also create - // contexts for inlined functions. - SafeToInline allowInline = SafeToInline::Yes; - std::function updateAllowInline = [&](Code* code) { - Visitor::check(code->entry, [&](Instruction* i) { - if (LdFun::Cast(i) || LdVar::Cast(i)) { - auto n = LdFun::Cast(i) ? LdFun::Cast(i)->varName - : LdVar::Cast(i)->varName; - if (!SafeBuiltinsList::forInlineByName(n)) { - allowInline = SafeToInline::No; - return false; - } + if (dontInline(inlineeCls)) + continue; + + enum SafeToInline { + Yes, + NeedsContext, + No, + }; + + // TODO: instead of blacklisting those, we could also create + // contexts for inlined functions. + SafeToInline allowInline = SafeToInline::Yes; + std::function updateAllowInline = [&](Code* code) { + Visitor::check(code->entry, [&](Instruction* i) { + if (LdFun::Cast(i) || LdVar::Cast(i)) { + auto n = LdFun::Cast(i) ? LdFun::Cast(i)->varName + : LdVar::Cast(i)->varName; + if (!SafeBuiltinsList::forInlineByName(n)) { + allowInline = SafeToInline::No; + return false; } - if (auto call = CallInstruction::CastCall(i)) { - if (auto trg = call->tryGetClsArg()) { - if (auto c = Const::Cast(trg)) { - if (TYPEOF(c->c()) == SPECIALSXP || - TYPEOF(c->c()) == BUILTINSXP) { - if (!SafeBuiltinsList::forInline( - c->c()->u.primsxp.offset)) { - allowInline = SafeToInline::No; - return false; - } + } + if (auto call = CallInstruction::CastCall(i)) { + if (auto trg = call->tryGetClsArg()) { + if (auto c = Const::Cast(trg)) { + if (TYPEOF(c->c()) == SPECIALSXP || + TYPEOF(c->c()) == BUILTINSXP) { + if (!SafeBuiltinsList::forInline( + c->c()->u.primsxp.offset)) { + allowInline = SafeToInline::No; + return false; } } } } - if (auto call = CallBuiltin::Cast(i)) { - if (!SafeBuiltinsList::forInline(call->builtinId)) { - allowInline = SafeToInline::No; - return false; - } - } - if (allowInline == SafeToInline::Yes && - i->mayObserveContext()) { - allowInline = SafeToInline::NeedsContext; - } - if (auto mk = MkArg::Cast(i)) { - updateAllowInline(mk->prom()); + } + if (auto call = CallBuiltin::Cast(i)) { + if (!SafeBuiltinsList::forInline(call->builtinId)) { + allowInline = SafeToInline::No; + return false; } - return true; - }); - }; - - size_t weight = inlinee->numNonDeoptInstrs(); - // The taken information of the call instruction tells us how - // many times a call was executed relative to function - // invocation. 0 means never, 1 means on every call, above 1 - // means more than once per call, ie. in a loop. - if (auto c = CallInstruction::CastCall(*it)) { - if (c->taken != CallInstruction::UnknownTaken && - !Parameter::INLINER_INLINE_UNLIKELY) { - // Policy: for calls taken about 80% the time the weight - // stays unchanged. Below it's increased and above it - // is decreased, but not more than 4x - double adjust = 1.25 * c->taken; - if (adjust > 4) - adjust = 4; - if (adjust < 0.2) - adjust = 0.2; - weight = (double)weight / adjust; - // Inline only small methods if we are getting close to - // the limit. - auto limit = (double)inlinee->numNonDeoptInstrs() / - (double)Parameter::INLINER_MAX_SIZE; - limit = (limit * 4) + 1; - weight *= limit; } + if (allowInline == SafeToInline::Yes && + i->mayObserveContext()) { + allowInline = SafeToInline::NeedsContext; + } + if (auto mk = MkArg::Cast(i)) { + updateAllowInline(mk->prom()); + } + return true; + }); + }; + + size_t weight = inlinee->numNonDeoptInstrs(); + // The taken information of the call instruction tells us how + // many times a call was executed relative to function + // invocation. 0 means never, 1 means on every call, above 1 + // means more than once per call, ie. in a loop. + if (auto c = CallInstruction::CastCall(*it)) { + if (c->taken != CallInstruction::UnknownTaken && + !Parameter::INLINER_INLINE_UNLIKELY) { + // Policy: for calls taken about 80% the time the weight + // stays unchanged. Below it's increased and above it + // is decreased, but not more than 4x + double adjust = 1.25 * c->taken; + if (adjust > 4) + adjust = 4; + if (adjust < 0.2) + adjust = 0.2; + weight = (double)weight / adjust; + // Inline only small methods if we are getting close to + // the limit. + auto limit = (double)inlinee->numNonDeoptInstrs() / + (double)Parameter::INLINER_MAX_SIZE; + limit = (limit * 4) + 1; + weight *= limit; } - auto env = Env::Cast(inlineeCls->closureEnv()); - if (env && env->rho && R_IsNamespaceEnv(env->rho)) { - auto expr = BODY_EXPR(inlineeCls->rirClosure()); - // Closure wrappers for internals - if (CAR(expr) == rir::symbol::Internal) - weight *= 0.6; - // those usually strongly benefit type - // inference, since they have a lot of case - // distinctions - static auto profitable = std::unordered_set( - {"matrix", "array", "vector", "cat"}); - if (profitable.count(inlineeCls->name())) - weight *= 0.4; - } - if (hasDotslistArg) + } + auto env = Env::Cast(inlineeCls->closureEnv()); + if (env && env->rho && R_IsNamespaceEnv(env->rho)) { + auto expr = BODY_EXPR(inlineeCls->rirClosure()); + // Closure wrappers for internals + if (CAR(expr) == rir::symbol::Internal) + weight *= 0.6; + // those usually strongly benefit type + // inference, since they have a lot of case + // distinctions + static auto profitable = std::unordered_set( + {"matrix", "array", "vector", "cat"}); + if (profitable.count(inlineeCls->name())) weight *= 0.4; - if (!(*it)->typeFeedback().type.isVoid() && - (*it)->typeFeedback().type.unboxable()) - weight *= 0.9; - - // No recursive inlining - if (inlinee->owner() == cls->owner() || - (callerFrameState && - callerFrameState->code == - inlinee->owner()->rirFunction()->body())) { - continue; - } else if (weight > Parameter::INLINER_MAX_INLINEE_SIZE) { - if (!inlineeCls->rirFunction()->flags.contains( - rir::Function::ForceInline) && - inlinee->numNonDeoptInstrs() > - Parameter::INLINER_MAX_INLINEE_SIZE * 4) - inlineeCls->rirFunction()->flags.set( - rir::Function::NotInlineable); + } + if (hasDotslistArg) + weight *= 0.4; + if (!(*it)->typeFeedback().type.isVoid() && + (*it)->typeFeedback().type.unboxable()) + weight *= 0.9; + + // No recursive inlining + if (inlinee->owner() == cls->owner() || + (callerFrameState && + callerFrameState->code == + inlinee->owner()->rirFunction()->body())) { + continue; + } else if (weight > Parameter::INLINER_MAX_INLINEE_SIZE) { + if (!inlineeCls->rirFunction()->flags.contains( + rir::Function::ForceInline) && + inlinee->numNonDeoptInstrs() > + Parameter::INLINER_MAX_INLINEE_SIZE * 4) + inlineeCls->rirFunction()->flags.set( + rir::Function::NotInlineable); + continue; + } else { + updateAllowInline(inlinee); + inlinee->eachPromise([&](Promise* p) { updateAllowInline(p); }); + if (allowInline == SafeToInline::No) { + inlineeCls->rirFunction()->flags.set( + rir::Function::NotInlineable); continue; - } else { - updateAllowInline(inlinee); - inlinee->eachPromise( - [&](Promise* p) { updateAllowInline(p); }); - if (allowInline == SafeToInline::No) { - inlineeCls->rirFunction()->flags.set( - rir::Function::NotInlineable); - continue; - } } + } - if (!inlineeCls->rirFunction()->flags.contains( - rir::Function::ForceInline)) - fuel--; - - cls->inlinees++; - - BB* split = BBTransform::split(cls->nextBBId++, bb, it, cls); - auto theCall = *split->begin(); - auto theCallInstruction = CallInstruction::CastCall(theCall); - std::vector arguments; - theCallInstruction->eachCallArg( - [&](Value* v) { arguments.push_back(v); }); - - // Clone the version - BB* copy = BBTransform::clone(inlinee->entry, code, cls); + if (!inlineeCls->rirFunction()->flags.contains( + rir::Function::ForceInline)) + fuel--; + + cls->inlinees++; + + BB* split = BBTransform::split(cls->nextBBId++, bb, it, cls); + auto theCall = *split->begin(); + auto theCallInstruction = CallInstruction::CastCall(theCall); + std::vector arguments; + theCallInstruction->eachCallArg( + [&](Value* v) { arguments.push_back(v); }); + + // Clone the version + BB* copy = BBTransform::clone(inlinee->entry, code, cls); + + bool needsEnvPatching = inlineeCls->closureEnv() != staticEnv; + + bool failedToInline = false; + bool hasNonLocalReturn = false; + bool hasReturn = false; + Visitor::run(copy, [&](BB* bb) { + auto ip = bb->begin(); + while (!failedToInline && ip != bb->end()) { + auto next = ip + 1; + auto ld = LdArg::Cast(*ip); + Instruction* i = *ip; + + if (auto l = LdFunctionEnv::Cast(i)) { + l->replaceUsesWith(staticEnv); + l->effects.reset(); + } - bool needsEnvPatching = inlineeCls->closureEnv() != staticEnv; + if (Return::Cast(i)) + hasReturn = true; + if (NonLocalReturn::Cast(i)) + hasNonLocalReturn = true; - bool failedToInline = false; - bool hasNonLocalReturn = false; - bool hasReturn = false; - Visitor::run(copy, [&](BB* bb) { - auto ip = bb->begin(); - while (!failedToInline && ip != bb->end()) { - auto next = ip + 1; - auto ld = LdArg::Cast(*ip); - Instruction* i = *ip; - - if (auto l = LdFunctionEnv::Cast(i)) { - l->replaceUsesWith(staticEnv); - l->effects.reset(); + if (auto sp = FrameState::Cast(i)) { + if (!callerFrameState) { + failedToInline = true; + return; } - if (Return::Cast(i)) - hasReturn = true; - if (NonLocalReturn::Cast(i)) - hasNonLocalReturn = true; + // When inlining a frameState we need to chain it + // with the frameStates after the call to the + // inlinee + if (!sp->next()) { + auto copyFromFs = callerFrameState; + auto cloneSp = + FrameState::Cast(copyFromFs->clone()); - if (auto sp = FrameState::Cast(i)) { - if (!callerFrameState) { - failedToInline = true; - return; - } + ip = bb->insert(ip, cloneSp); + sp->next(cloneSp); - // When inlining a frameState we need to chain it - // with the frameStates after the call to the - // inlinee - if (!sp->next()) { - auto copyFromFs = callerFrameState; - auto cloneSp = - FrameState::Cast(copyFromFs->clone()); + size_t created = 1; + while (copyFromFs->next()) { + assert(copyFromFs->next() == cloneSp->next()); + copyFromFs = copyFromFs->next(); + auto prevClone = cloneSp; + cloneSp = FrameState::Cast(copyFromFs->clone()); ip = bb->insert(ip, cloneSp); - sp->next(cloneSp); - - size_t created = 1; - while (copyFromFs->next()) { - assert(copyFromFs->next() == - cloneSp->next()); - copyFromFs = copyFromFs->next(); - auto prevClone = cloneSp; - cloneSp = - FrameState::Cast(copyFromFs->clone()); - - ip = bb->insert(ip, cloneSp); - created++; + created++; - prevClone->updateNext(cloneSp); - } - - next = ip + created + 1; + prevClone->updateNext(cloneSp); } + + next = ip + created + 1; } - // If the inlining resolved some env, we need to - // update. For example this happens if we inline an - // inner version. Then the lexical env is the current - // versions env. - if (needsEnvPatching && i->hasEnv() && - i->env() == inlineeCls->closureEnv()) { - i->env(staticEnv); - } + } + // If the inlining resolved some env, we need to + // update. For example this happens if we inline an + // inner version. Then the lexical env is the current + // versions env. + if (needsEnvPatching && i->hasEnv() && + i->env() == inlineeCls->closureEnv()) { + i->env(staticEnv); + } - // If we inline without context, then we need to update - // the mkEnv instructions in the inlinee, such that - // they do not update the (non-existing) context. - if (allowInline != SafeToInline::NeedsContext) { - if (auto mk = MkEnv::Cast(i)) { - mk->context--; - } + // If we inline without context, then we need to update + // the mkEnv instructions in the inlinee, such that + // they do not update the (non-existing) context. + if (allowInline != SafeToInline::NeedsContext) { + if (auto mk = MkEnv::Cast(i)) { + mk->context--; } + } - if (ld) { - Value* a = (ld->pos < arguments.size()) - ? arguments[ld->pos] - : MissingArg::instance(); - if (auto mk = MkArg::Cast(a)) { - if (!ld->type.maybePromiseWrapped()) { - // This load already expects to load an - // eager value. We can just discard the - // promise altogether. - assert(mk->isEager()); - a = mk->eagerArg(); - } else { - // We need to cast from a promise to a lazy - // value - auto type = ld->type.notMissing(); - if (mk->isEager()) { - auto inType = mk->eagerArg()->type; - type = - inType.forced().orPromiseWrapped(); - if (!inType.maybeMissing()) - type = type.notMissing(); - } - auto cast = new CastType( - a, CastType::Upcast, RType::prom, type); - ip = bb->insert(ip + 1, cast); - ip--; - a = cast; - } - } - if (a == MissingArg::instance()) { - ld->replaceUsesWith( - a, [&](Instruction* usage, size_t arg) { - if (auto mk = MkEnv::Cast(usage)) - mk->missing[arg] = true; - }); + if (ld) { + Value* a = (ld->pos < arguments.size()) + ? arguments[ld->pos] + : MissingArg::instance(); + if (auto mk = MkArg::Cast(a)) { + if (!ld->type.maybePromiseWrapped()) { + // This load already expects to load an + // eager value. We can just discard the + // promise altogether. + assert(mk->isEager()); + a = mk->eagerArg(); } else { - ld->replaceUsesWith(a); + // We need to cast from a promise to a lazy + // value + auto type = ld->type.notMissing(); + if (mk->isEager()) { + auto inType = mk->eagerArg()->type; + type = inType.forced().orPromiseWrapped(); + if (!inType.maybeMissing()) + type = type.notMissing(); + } + auto cast = new CastType(a, CastType::Upcast, + RType::prom, type); + ip = bb->insert(ip + 1, cast); + ip--; + a = cast; } - next = bb->remove(ip); } - ip = next; + if (a == MissingArg::instance()) { + ld->replaceUsesWith( + a, [&](Instruction* usage, size_t arg) { + if (auto mk = MkEnv::Cast(usage)) + mk->missing[arg] = true; + }); + } else { + ld->replaceUsesWith(a); + } + next = bb->remove(ip); } - }); + ip = next; + } + }); + + if (!hasReturn && !hasNonLocalReturn) + failedToInline = true; + + if (failedToInline) { + std::vector toDel; + Visitor::run(copy, [&](BB* bb) { toDel.push_back(bb); }); + for (auto bb : toDel) + delete bb; + bb->overrideNext(split); + inlineeCls->rirFunction()->flags.set( + rir::Function::NotInlineable); + } else { + anyChange = true; + Checkpoint* cpAtCall = nullptr; + { + AvailableCheckpoints cp(cls, code, log); + cpAtCall = cp.at(theCall); + } - if (!hasReturn && !hasNonLocalReturn) - failedToInline = true; + bb->overrideNext(copy); - if (failedToInline) { - std::vector toDel; - Visitor::run(copy, [&](BB* bb) { toDel.push_back(bb); }); - for (auto bb : toDel) - delete bb; - bb->overrideNext(split); - inlineeCls->rirFunction()->flags.set( - rir::Function::NotInlineable); - } else { - anyChange = true; - Checkpoint* cpAtCall = nullptr; - { - AvailableCheckpoints cp(cls, code, log); - cpAtCall = cp.at(theCall); - } + // Copy over promises used by the inner version + std::vector copiedPromise(false); + std::vector newPromId; + copiedPromise.resize(inlinee->promises().size(), false); + newPromId.resize(inlinee->promises().size()); + Visitor::run(copy, [&](BB* bb) { + auto it = bb->begin(); + while (it != bb->end()) { + MkArg* mk = MkArg::Cast(*it); + it++; + if (!mk) + continue; - bb->overrideNext(copy); - - // Copy over promises used by the inner version - std::vector copiedPromise(false); - std::vector newPromId; - copiedPromise.resize(inlinee->promises().size(), false); - newPromId.resize(inlinee->promises().size()); - Visitor::run(copy, [&](BB* bb) { - auto it = bb->begin(); - while (it != bb->end()) { - MkArg* mk = MkArg::Cast(*it); - it++; - if (!mk) - continue; - - size_t id = mk->prom()->id; - if (mk->prom()->owner == inlinee) { - assert(id < copiedPromise.size()); - if (copiedPromise[id]) { - mk->updatePromise( - cls->promises().at(newPromId[id])); - } else { - Promise* clone = - cls->createProm(mk->prom()->rirSrc()); - BB* promCopy = BBTransform::clone( - mk->prom()->entry, clone, cls); - clone->entry = promCopy; - newPromId[id] = clone->id; - copiedPromise[id] = true; - mk->updatePromise(clone); - } - } - } - }); - - auto inlineeRes = BBTransform::forInline( - copy, split, inlineeCls->closureEnv(), cpAtCall); - - assert(inlineeRes != Tombstone::unreachable()); - - if (allowInline == SafeToInline::NeedsContext) { - Value* op = nullptr; - auto prologue = copy; - copy = BBTransform::split(cls->nextBBId++, copy, - copy->begin(), cls); - assert(prologue->isEmpty()); - if (auto call = Call::Cast(theCall)) { - op = call->cls(); - } else if (auto call = StaticCall::Cast(theCall)) { - if (call->runtimeClosure() != - Tombstone::closure()) { - op = call->runtimeClosure(); + size_t id = mk->prom()->id; + if (mk->prom()->owner == inlinee) { + assert(id < copiedPromise.size()); + if (copiedPromise[id]) { + mk->updatePromise( + cls->promises().at(newPromId[id])); } else { - op = cmp.module->c(call->cls()->rirClosure()); + Promise* clone = + cls->createProm(mk->prom()->rirSrc()); + BB* promCopy = BBTransform::clone( + mk->prom()->entry, clone, cls); + clone->entry = promCopy; + newPromId[id] = clone->id; + copiedPromise[id] = true; + mk->updatePromise(clone); } } - assert(op); - auto ast = - cmp.module->c(rir::Pool::get(theCall->srcIdx)); - auto ctx = new PushContext(ast, op, theCallInstruction, - theCall->env()); - prologue->append(ctx); - - auto popc = new PopContext(inlineeRes, ctx); - split->insert(split->begin() + 1, popc); - popc->type = popc->type & theCall->type; - popc->updateTypeAndEffects(); - - if (hasNonLocalReturn) { - assert(split->predecessors().empty()); - assert(copy->hasSinglePred()); - // No normal return, this means that pop-context - // looks unreachable, even though it is reachable - // through non-local returns. - auto fake1 = new BB(cls, cls->nextBBId++); - // avoids critical edge - auto fake2 = new BB(cls, cls->nextBBId++); - assert(((const BB*)prologue)->next() == copy); - prologue->overrideNext(fake1); - fake1->append(new Branch(OpaqueTrue::instance())); - fake1->setSuccessors({fake2, split}); - fake2->setSuccessors({copy}); - } - inlineeRes = popc; } + }); - theCall->replaceUsesWith(inlineeRes); - - // Remove the call instruction - split->remove(split->begin()); + auto inlineeRes = BBTransform::forInline( + copy, split, inlineeCls->closureEnv(), cpAtCall); + + assert(inlineeRes != Tombstone::unreachable()); + + if (allowInline == SafeToInline::NeedsContext) { + Value* op = nullptr; + auto prologue = copy; + copy = BBTransform::split(cls->nextBBId++, copy, + copy->begin(), cls); + assert(prologue->isEmpty()); + if (auto call = Call::Cast(theCall)) { + op = call->cls(); + } else if (auto call = StaticCall::Cast(theCall)) { + if (call->runtimeClosure() != Tombstone::closure()) { + op = call->runtimeClosure(); + } else { + op = cmp.module->c(call->cls()->rirClosure()); + } + } + assert(op); + auto ast = cmp.module->c(rir::Pool::get(theCall->srcIdx)); + auto ctx = new PushContext(ast, op, theCallInstruction, + theCall->env()); + prologue->append(ctx); + + auto popc = new PopContext(inlineeRes, ctx); + split->insert(split->begin() + 1, popc); + popc->type = popc->type & theCall->type; + popc->updateTypeAndEffects(); + + if (hasNonLocalReturn) { + assert(split->predecessors().empty()); + assert(copy->hasSinglePred()); + // No normal return, this means that pop-context + // looks unreachable, even though it is reachable + // through non-local returns. + auto fake1 = new BB(cls, cls->nextBBId++); + // avoids critical edge + auto fake2 = new BB(cls, cls->nextBBId++); + assert(((const BB*)prologue)->next() == copy); + prologue->overrideNext(fake1); + fake1->append(new Branch(OpaqueTrue::instance())); + fake1->setSuccessors({fake2, split}); + fake2->setSuccessors({copy}); + } + inlineeRes = popc; } - bb = split; - it = split->begin(); + theCall->replaceUsesWith(inlineeRes); - // Can happen if split only contained the call instruction - if (it == split->end()) - break; + // Remove the call instruction + split->remove(split->begin()); } - }); + + bb = split; + it = split->begin(); + + // Can happen if split only contained the call instruction + if (it == split->end()) + break; + } + }); return anyChange; } -// TODO: maybe implement something more resonable to pass in those constants. +// TODO: maybe implement something more reasonable to pass in those constants. // For now it seems a simple env variable is just fine. - size_t Parameter::INLINER_MAX_SIZE = - getenv("PIR_INLINER_MAX_SIZE") ? atoi(getenv("PIR_INLINER_MAX_SIZE")) - : 2500; - size_t Parameter::INLINER_MAX_INLINEE_SIZE = - getenv("PIR_INLINER_MAX_INLINEE_SIZE") - ? atoi(getenv("PIR_INLINER_MAX_INLINEE_SIZE")) - : 90; - size_t Parameter::INLINER_INITIAL_FUEL = - getenv("PIR_INLINER_INITIAL_FUEL") - ? atoi(getenv("PIR_INLINER_INITIAL_FUEL")) - : 15; - size_t Parameter::INLINER_INLINE_UNLIKELY = - getenv("PIR_INLINER_INLINE_UNLIKELY") - ? atoi(getenv("PIR_INLINER_INLINE_UNLIKELY")) - : 0; +size_t Parameter::INLINER_MAX_SIZE = getenv("PIR_INLINER_MAX_SIZE") + ? atoi(getenv("PIR_INLINER_MAX_SIZE")) + : 2500; +size_t Parameter::INLINER_MAX_INLINEE_SIZE = + getenv("PIR_INLINER_MAX_INLINEE_SIZE") + ? atoi(getenv("PIR_INLINER_MAX_INLINEE_SIZE")) + : 90; +size_t Parameter::INLINER_INITIAL_FUEL = + getenv("PIR_INLINER_INITIAL_FUEL") + ? atoi(getenv("PIR_INLINER_INITIAL_FUEL")) + : 15; +size_t Parameter::INLINER_INLINE_UNLIKELY = + getenv("PIR_INLINER_INLINE_UNLIKELY") + ? atoi(getenv("PIR_INLINER_INLINE_UNLIKELY")) + : 0; } // namespace pir } // namespace rir diff --git a/rir/src/compiler/opt/scope_resolution.cpp b/rir/src/compiler/opt/scope_resolution.cpp index 41a619cb6..9ce2c44ef 100644 --- a/rir/src/compiler/opt/scope_resolution.cpp +++ b/rir/src/compiler/opt/scope_resolution.cpp @@ -478,10 +478,9 @@ bool ScopeResolution::apply(Compiler& cmp, ClosureVersion* cls, Code* code, if (mk->context) { auto diff = contexts.before(deoptEnv) - .context() - - contexts.before(mk).context(); - deoptEnv->context = - mk->context + diff; + .numContexts() - + contexts.before(mk).numContexts(); + deoptEnv->context = mk->context + diff; } else { deoptEnv->context = 0; } diff --git a/rir/src/compiler/pir/builder.cpp b/rir/src/compiler/pir/builder.cpp index 7e6403ba6..e72412ae2 100644 --- a/rir/src/compiler/pir/builder.cpp +++ b/rir/src/compiler/pir/builder.cpp @@ -123,8 +123,7 @@ Builder::Builder(Continuation* cnt, Value* closureEnv) e++; i++; } - auto mkenv = new MkEnv(closureEnv, names, args.data()); - mkenv->missing = miss; + auto mkenv = new MkEnv(closureEnv, names, args.data(), miss); auto rirCode = cnt->owner()->rirFunction()->body(); mkenv->updateTypeFeedback().feedbackOrigin.srcCode(rirCode); diff --git a/rir/src/compiler/pir/instruction.h b/rir/src/compiler/pir/instruction.h index f8045c7d1..bb4b8fef8 100644 --- a/rir/src/compiler/pir/instruction.h +++ b/rir/src/compiler/pir/instruction.h @@ -2432,7 +2432,7 @@ class VLIE(MkEnv, Effect::LeaksArg) { MutableLocalVarIt; inline void eachLocalVar(MutableLocalVarIt it) { - for (size_t i = 0; i < envSlot(); ++i) { + for (size_t i = 0; i < nLocals(); ++i) { bool m = missing[i]; it(varName[i], arg(i), m); missing[i] = m; @@ -2440,13 +2440,16 @@ class VLIE(MkEnv, Effect::LeaksArg) { } inline void eachLocalVar(LocalVarIt it) const { - for (size_t i = 0; i < envSlot(); ++i) + for (size_t i = 0; i < nLocals(); ++i) { it(varName[i], arg(i).val(), missing[i]); + } } inline void eachLocalVarRev(LocalVarIt it) const { - for (long i = envSlot() - 1; i >= 0; --i) - it(varName[i], arg(i).val(), missing[i]); + for (size_t i = 0; i < nLocals(); ++i) { + size_t ri = nLocals() - i - 1; + it(varName[ri], arg(ri).val(), missing[ri]); + } } MkEnv(Value* lexicalEnv, const std::vector& names, Value** args, @@ -2482,7 +2485,7 @@ class VLIE(MkEnv, Effect::LeaksArg) { void printEnv(std::ostream& out, bool tty) const override final {} std::string name() const override { return stub ? "(MkEnv)" : "MkEnv"; } - size_t nLocals() { return nargs() - 1; } + size_t nLocals() const { return nargs() - 1; } int minReferenceCount() const override { return MAX_REFCOUNT; } @@ -2643,12 +2646,12 @@ class VLI(Phi, Effects::None()) { const std::vector& inputs() { return input; } void removeInputs(const std::unordered_set& del); - typedef std::function PhiArgumentIterator; + typedef std::function PhiArgumentIterator; void eachArg(const PhiArgumentIterator& it) const { for (size_t i = 0; i < nargs(); ++i) it(input[i], arg(i).val()); } - typedef std::function MutablePhiArgumentIterator; + typedef std::function MutablePhiArgumentIterator; void eachArg(const MutablePhiArgumentIterator& it) { for (size_t i = 0; i < nargs(); ++i) it(input[i], arg(i)); diff --git a/rir/src/compiler/pir/singleton_values.h b/rir/src/compiler/pir/singleton_values.h index 4bf1f9136..2dc959c6f 100644 --- a/rir/src/compiler/pir/singleton_values.h +++ b/rir/src/compiler/pir/singleton_values.h @@ -156,6 +156,7 @@ class Tombstone : public Value { private: explicit Tombstone(PirType t) : Value(t, Tag::Tombstone) {} }; + } // namespace pir } // namespace rir diff --git a/rir/src/compiler/pir/tag.h b/rir/src/compiler/pir/tag.h index 934313d52..3c8dce3e9 100644 --- a/rir/src/compiler/pir/tag.h +++ b/rir/src/compiler/pir/tag.h @@ -20,12 +20,13 @@ enum class Tag : uint8_t { COMPILER_INSTRUCTIONS(V) #undef V #define V(I) I, - COMPILER_VALUES(V) + COMPILER_VALUES(V) #undef V }; const char* tagToStr(Tag t); -} -} + +} // namespace pir +} // namespace rir #endif diff --git a/rir/src/compiler/rir2pir/rir2pir.cpp b/rir/src/compiler/rir2pir/rir2pir.cpp index ead3062fd..d645922b0 100644 --- a/rir/src/compiler/rir2pir/rir2pir.cpp +++ b/rir/src/compiler/rir2pir/rir2pir.cpp @@ -1691,7 +1691,7 @@ Value* Rir2Pir::tryTranslate(rir::Code* srcCode, Builder& insert, Opcode* start, } else { BB* merge = insert.createBB(); insert.enterBB(merge); - Phi* phi = insert(new Phi()); + auto phi = insert(new Phi); for (auto r : results) { r.first->setNext(merge); phi->addInput(r.first, r.second); diff --git a/rir/src/compiler/util/bb_transform.cpp b/rir/src/compiler/util/bb_transform.cpp index 735c96faa..0c30966c0 100644 --- a/rir/src/compiler/util/bb_transform.cpp +++ b/rir/src/compiler/util/bb_transform.cpp @@ -125,7 +125,7 @@ Value* BBTransform::forInline(BB* inlinee, BB* splice, Value* context, continue; } - // This is the first cp of the inlinee, lets replace it with the + // This is the first CP of the inlinee, let's replace it with the // outer CP if (pos->isCheckpoint()) { auto cp = Checkpoint::Cast(pos->last()); @@ -205,7 +205,7 @@ BB* BBTransform::lowerExpect(Module* m, Code* code, BB* srcBlock, auto d = Deopt::Cast(deoptBlock_->last()); if (d->deoptReason() == DeoptReasonWrapper::unknown()) { auto newDr = new Phi(NativeType::deoptReason); - auto newDt = new Phi(); + auto newDt = new Phi; deoptBlock_->insert(deoptBlock_->begin(), newDr); deoptBlock_->insert(deoptBlock_->begin(), newDt); d->setDeoptReason(newDr, newDt); diff --git a/rir/src/compiler/util/bb_transform.h b/rir/src/compiler/util/bb_transform.h index e5222e377..a21c4d56b 100644 --- a/rir/src/compiler/util/bb_transform.h +++ b/rir/src/compiler/util/bb_transform.h @@ -1,11 +1,11 @@ #ifndef BB_TRANSFORM_H #define BB_TRANSFORM_H -#include "../pir/bb.h" -#include "../pir/pir.h" -#include "../pir/values.h" #include "compiler/analysis/cfg.h" #include "compiler/compiler.h" +#include "compiler/pir/bb.h" +#include "compiler/pir/pir.h" +#include "compiler/pir/values.h" #include "compiler/rir2pir/rir2pir.h" #include "runtime/TypeFeedback.h" From 1f8026ab5e997226aca33f488087b84f3b14a5a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Mon, 13 Jun 2022 14:31:34 +0000 Subject: [PATCH 2/8] more cleanup --- rir/src/compiler/backend.cpp | 16 ++++++++-------- rir/src/compiler/native/builtins.cpp | 3 +-- rir/src/compiler/util/bb_transform.cpp | 2 +- rir/src/compiler/util/bb_transform.h | 2 +- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/rir/src/compiler/backend.cpp b/rir/src/compiler/backend.cpp index 4da6f2976..ab73fe301 100644 --- a/rir/src/compiler/backend.cpp +++ b/rir/src/compiler/backend.cpp @@ -184,31 +184,31 @@ static void lower(Module* module, Code* code) { Tombstone::framestate(), 0)); next = it + 2; } - } else if (auto expect = Assume::Cast(*it)) { - if (expect->triviallyHolds()) { + } else if (auto assume = Assume::Cast(*it)) { + if (assume->triviallyHolds()) { next = bb->remove(it); } else { - auto expectation = expect->assumeTrue; + auto expectation = assume->assumeTrue; std::string debugMessage; if (Parameter::DEBUG_DEOPTS) { debugMessage = "DEOPT, assumption "; { std::stringstream dump; if (auto i = - Instruction::Cast(expect->condition())) { + Instruction::Cast(assume->condition())) { dump << "\n"; i->printRecursive(dump, 4); dump << "\n"; } else { - expect->condition()->printRef(dump); + assume->condition()->printRef(dump); } debugMessage += dump.str(); } debugMessage += " failed\n"; } - BBTransform::lowerExpect( - module, code, bb, it, expect, expectation, - expect->checkpoint()->bb()->falseBranch(), + BBTransform::lowerAssume( + module, code, bb, it, assume, expectation, + assume->checkpoint()->deoptBranch(), debugMessage); // lowerExpect splits the bb from current position. There // remains nothing to process. Breaking seems more robust diff --git a/rir/src/compiler/native/builtins.cpp b/rir/src/compiler/native/builtins.cpp index b2186872c..a6497c76e 100644 --- a/rir/src/compiler/native/builtins.cpp +++ b/rir/src/compiler/native/builtins.cpp @@ -2163,8 +2163,7 @@ void initClosureContextImpl(ArglistOrder::CallId callId, rir::Code* c, SEXP ast, } static void endClosureContextImpl(RCNTXT* cntxt, SEXP result) { - cntxt->returnValue = result; - Rf_endcontext(cntxt); + endClosureContext(cntxt, result); } int ncolsImpl(SEXP v) { return getMatrixDim(v).col; } diff --git a/rir/src/compiler/util/bb_transform.cpp b/rir/src/compiler/util/bb_transform.cpp index 0c30966c0..8d3ddc886 100644 --- a/rir/src/compiler/util/bb_transform.cpp +++ b/rir/src/compiler/util/bb_transform.cpp @@ -172,7 +172,7 @@ Value* BBTransform::forInline(BB* inlinee, BB* splice, Value* context, return found; } -BB* BBTransform::lowerExpect(Module* m, Code* code, BB* srcBlock, +BB* BBTransform::lowerAssume(Module* m, Code* code, BB* srcBlock, BB::Instrs::iterator position, Assume* assume, bool condition, BB* deoptBlock_, const std::string& debugMessage) { diff --git a/rir/src/compiler/util/bb_transform.h b/rir/src/compiler/util/bb_transform.h index a21c4d56b..fe0babd27 100644 --- a/rir/src/compiler/util/bb_transform.h +++ b/rir/src/compiler/util/bb_transform.h @@ -28,7 +28,7 @@ class BBTransform { Code* target); static Value* forInline(BB* inlinee, BB* cont, Value* context, Checkpoint* entryCp); - static BB* lowerExpect(Module* m, Code* closure, BB* src, + static BB* lowerAssume(Module* m, Code* closure, BB* src, BB::Instrs::iterator position, Assume* assume, bool condition, BB* deoptBlock, const std::string& debugMesage); From ab672a96400926b948d9ddf91e0cd070ab540271 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Mon, 13 Jun 2022 14:40:30 +0000 Subject: [PATCH 3/8] Remove extra contexts while deopting Because of an optimization where we replace the first checkpoint in an inlinee by the checkpoint at the callsite it is possible to take the outer deopt exit from within the inlinee. In case the inlinee needs its own context we need to pop it, otherwise the context offsets in MkEnv instructions in the deopt block would modify the wrong frame. ``` a = checkpoint -> [deopt with (mkenv ... context 1)] ... c = push_context .... assume ..., a -> we have to pop c ``` The other option would be to make the context field in MkEnvs a PIR variable and then set it to the correct value based on where we come from, however, this seems easier. --- rir/src/compiler/backend.cpp | 26 ++++++++++++++++--- .../compiler/native/lower_function_llvm.cpp | 21 +++++++++++++++ rir/src/compiler/pir/instruction.h | 5 ++++ rir/src/compiler/pir/instruction_list.h | 1 + rir/src/compiler/util/bb_transform.cpp | 8 ++++-- rir/src/compiler/util/bb_transform.h | 4 +-- 6 files changed, 57 insertions(+), 8 deletions(-) diff --git a/rir/src/compiler/backend.cpp b/rir/src/compiler/backend.cpp index ab73fe301..e85d012bd 100644 --- a/rir/src/compiler/backend.cpp +++ b/rir/src/compiler/backend.cpp @@ -1,5 +1,6 @@ #include "backend.h" #include "R/BuiltinIds.h" +#include "analysis/context_stack.h" #include "analysis/dead.h" #include "bc/CodeStream.h" #include "bc/CodeVerifier.h" @@ -106,11 +107,28 @@ static void approximateNeedsLdVarForUpdate( }); } -static void lower(Module* module, Code* code) { +static void lower(Module* module, ClosureVersion* cls, Code* code, + AbstractLog& log) { DeadInstructions representAsReal( code, 1, Effects::Any(), DeadInstructions::IgnoreUsesThatDontObserveIntVsReal); + // If we take a deopt that's in between a PushContext/PopContext pair but + // whose Checkpoint is not, we have to remove the extra context(s). We emit + // DropContext instructions for this while lowering the Assume to a branch. + ContextStack cs(cls, code, log); + std::unordered_map nDropContexts; + Visitor::run(code->entry, [&](Instruction* i) { + if (auto a = Assume::Cast(i)) { + auto beforeA = cs.before(a).numContexts(); + auto beforeCp = cs.before(a->checkpoint()).numContexts(); + + assert(nDropContexts.count(a) == 0); + assert(beforeCp <= beforeA); + nDropContexts[a] = beforeA - beforeCp; + } + }); + Visitor::runPostChange(code->entry, [&](BB* bb) { auto it = bb->begin(); while (it != bb->end()) { @@ -207,8 +225,8 @@ static void lower(Module* module, Code* code) { debugMessage += " failed\n"; } BBTransform::lowerAssume( - module, code, bb, it, assume, expectation, - assume->checkpoint()->deoptBranch(), + module, code, bb, it, assume, nDropContexts.at(assume), + expectation, assume->checkpoint()->deoptBranch(), debugMessage); // lowerExpect splits the bb from current position. There // remains nothing to process. Breaking seems more robust @@ -324,7 +342,7 @@ rir::Function* Backend::doCompile(ClosureVersion* cls, ClosureLog& log) { std::function lowerAndScanForPromises = [&](Code* c) { if (promMap.count(c)) return; - lower(module, c); + lower(module, cls, c, log); toCSSA(module, c); log.CSSA(c); #ifdef FULLVERIFIER diff --git a/rir/src/compiler/native/lower_function_llvm.cpp b/rir/src/compiler/native/lower_function_llvm.cpp index 622779a91..2c3bb9019 100644 --- a/rir/src/compiler/native/lower_function_llvm.cpp +++ b/rir/src/compiler/native/lower_function_llvm.cpp @@ -2352,6 +2352,27 @@ void LowerFunctionLLVM::compile() { break; } + case Tag::DropContext: { + auto globalContextPtrAddr = + convertToPointer(&R_GlobalContext, t::RCNTXT_ptr); + auto globalContextPtr = + builder.CreateLoad(globalContextPtrAddr); + auto callflagAddr = + builder.CreateGEP(globalContextPtr, {c(0), c(1)}); + auto callflag = builder.CreateLoad(callflagAddr); + insn_assert( + builder.CreateICmpNE( + builder.CreateAnd(callflag, c(CTXT_FUNCTION)), c(0)), + "Expected R_GlobalContext to be a closure context"); + // R_GlobalContext = R_GlobalContext->nextcontext + auto nextcontextAddr = + builder.CreateGEP(globalContextPtr, {c(0), c(0)}); + auto nextcontext = builder.CreateLoad(nextcontextAddr); + builder.CreateStore(nextcontext, globalContextPtrAddr); + inPushContext--; + break; + } + case Tag::CastType: { auto in = i->arg(0).val(); if (!variables_.count(i)) diff --git a/rir/src/compiler/pir/instruction.h b/rir/src/compiler/pir/instruction.h index bb4b8fef8..f186cca50 100644 --- a/rir/src/compiler/pir/instruction.h +++ b/rir/src/compiler/pir/instruction.h @@ -2562,6 +2562,11 @@ class FLI(PopContext, 2, Effect::ChangesContexts) { Value* result() const { return arg<0>().val(); } }; +class FLI(DropContext, 0, Effect::ChangesContexts) { + public: + DropContext() : FixedLenInstruction(PirType::voyd()) {} +}; + class FLIE(LdDots, 1, Effect::ReadsEnv) { public: std::vector names; diff --git a/rir/src/compiler/pir/instruction_list.h b/rir/src/compiler/pir/instruction_list.h index 8a52b3ee3..7be0165e2 100644 --- a/rir/src/compiler/pir/instruction_list.h +++ b/rir/src/compiler/pir/instruction_list.h @@ -103,6 +103,7 @@ V(MaterializeEnv) \ V(PushContext) \ V(PopContext) \ + V(DropContext) \ V(LdFunctionEnv) \ V(Inc) \ V(Is) \ diff --git a/rir/src/compiler/util/bb_transform.cpp b/rir/src/compiler/util/bb_transform.cpp index 8d3ddc886..7682c5c2d 100644 --- a/rir/src/compiler/util/bb_transform.cpp +++ b/rir/src/compiler/util/bb_transform.cpp @@ -174,8 +174,8 @@ Value* BBTransform::forInline(BB* inlinee, BB* splice, Value* context, BB* BBTransform::lowerAssume(Module* m, Code* code, BB* srcBlock, BB::Instrs::iterator position, Assume* assume, - bool condition, BB* deoptBlock_, - const std::string& debugMessage) { + size_t nDropContexts, bool condition, + BB* deoptBlock_, const std::string& debugMessage) { auto split = BBTransform::split(code->nextBBId++, srcBlock, position + 1, code); @@ -197,6 +197,10 @@ BB* BBTransform::lowerAssume(Module* m, Code* code, BB* srcBlock, Tombstone::framestate(), 0)); } + while (nDropContexts--) { + deoptBlock->append(new DropContext()); + } + auto deoptReason = m->deoptReasonValue(assume->reason); auto deoptTrigger = assume->valueUnderTest(); if (!deoptTrigger) diff --git a/rir/src/compiler/util/bb_transform.h b/rir/src/compiler/util/bb_transform.h index fe0babd27..a2c5dbc45 100644 --- a/rir/src/compiler/util/bb_transform.h +++ b/rir/src/compiler/util/bb_transform.h @@ -30,8 +30,8 @@ class BBTransform { Checkpoint* entryCp); static BB* lowerAssume(Module* m, Code* closure, BB* src, BB::Instrs::iterator position, Assume* assume, - bool condition, BB* deoptBlock, - const std::string& debugMesage); + size_t nDropContexts, bool condition, + BB* deoptBlock_, const std::string& debugMesage); static void insertAssume(Instruction* condition, bool assumePositive, Checkpoint* cp, const FeedbackOrigin& origin, DeoptReason::Reason reason, BB* bb, From ca8948e9e5c55da16f4a0972f39b6ac2593bb9a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 14 Jun 2022 09:26:54 +0000 Subject: [PATCH 4/8] cleanup --- .../compiler/native/lower_function_llvm.cpp | 1 + rir/src/compiler/rir2pir/rir2pir.cpp | 2 +- rir/src/interpreter/interp.cpp | 84 +++++++++---------- 3 files changed, 44 insertions(+), 43 deletions(-) diff --git a/rir/src/compiler/native/lower_function_llvm.cpp b/rir/src/compiler/native/lower_function_llvm.cpp index 2c3bb9019..c315ae1fc 100644 --- a/rir/src/compiler/native/lower_function_llvm.cpp +++ b/rir/src/compiler/native/lower_function_llvm.cpp @@ -2112,6 +2112,7 @@ void LowerFunctionLLVM::compile() { stack({container(paramCode())}); additionalStackSlots++; } + { SmallSet> bindings; Visitor::run(code->entry, [&](Instruction* i) { diff --git a/rir/src/compiler/rir2pir/rir2pir.cpp b/rir/src/compiler/rir2pir/rir2pir.cpp index d645922b0..f3f569a74 100644 --- a/rir/src/compiler/rir2pir/rir2pir.cpp +++ b/rir/src/compiler/rir2pir/rir2pir.cpp @@ -811,7 +811,7 @@ bool Rir2Pir::compileBC(const BC& bc, Opcode* pos, Opcode* nextPos, break; } - // Specialcase for calling usemethod, the first argument is eager. + // Specialcase for calling UseMethod, the first argument is eager. // This helps determine the object type of the caller. { auto dt = DispatchTable::unpack(BODY(ti.monomorphic)); diff --git a/rir/src/interpreter/interp.cpp b/rir/src/interpreter/interp.cpp index fb69b0d45..c081294e0 100644 --- a/rir/src/interpreter/interp.cpp +++ b/rir/src/interpreter/interp.cpp @@ -34,41 +34,40 @@ static SEXP evalRirCode(Code* c, SEXP env, const CallContext* callContext, Opcode* initialPc = nullptr, BindingCache* cache = nullptr); +void printStack(int n) { + int sz = ostack_length(); + std::cout << "ostack (length = " << sz << ")\n"; + if (n > sz) + n = sz; + for (int i = n; i > 0; i--) { + auto cell = ostack_cell_at(i - 1); + auto sexp = cell->u.sxpval; + std::cout << "* ostack[" << (sz - i) << "] = "; + if (cell->tag == 0 && sexp) { + std::cout << Print::dumpSexp(sexp, 100); + } else { + std::cout << "{ tag = " << cell->tag << ", flags = " << cell->flags + << ", u = { ival = " << cell->u.ival + << ", dval = " << cell->u.dval << ", sxpval = " << sexp + << " } }"; + } + std::cout << "\n"; + } + std::cout.flush(); +} + // #define PRINT_INTERP // #define PRINT_STACK_SIZE 10 #ifdef PRINT_INTERP static void printInterp(Opcode* pc, Code* c) { #ifdef PRINT_STACK_SIZE -#define INTSEQSXP 9999 // Prevent printing instructions (and recursing) while printing stack static bool printingStackSize = false; if (printingStackSize) return; - // Print stack printingStackSize = true; - std::cout << "#; Stack:\n"; - for (int i = 0;; i++) { - auto typ = ostack_cell_at(i)->tag; - SEXP sexp = ostack_at(i); - if (sexp == nullptr || ostack_length() - i == 0) - break; - else if (i == PRINT_STACK_SIZE) { - std::cout << " ...\n"; - break; - } - if (typ == 0) { - std::cout << " >>> " << Print::dumpSexp(sexp) << " <<<\n"; - } else if (typ == INTSXP || typ == LGLSXP) { - std::cout << " int/lgl >>> " << ostack_cell_at(i)->u.ival - << " <<<\n"; - } else if (typ == REALSXP) { - std::cout << " real >>> " << ostack_cell_at(i)->u.dval - << " <<<\n"; - } else if (typ == INTSEQSXP) { - std::cout << " intseq >>> " << Print::dumpSexp(sexp) << " <<<\n"; - } - } + printStack(PRINT_STACK_SIZE); printingStackSize = false; #endif // Print source @@ -82,7 +81,6 @@ static void printInterp(Opcode* pc, Code* c) { std::cout << "#"; bc.print(std::cout); } - static void printLastop() { std::cout << "> lastop\n"; } #endif @@ -276,27 +274,27 @@ static void __listAppend(SEXP* front, SEXP* last, SEXP value, SEXP name) { #pragma GCC diagnostic ignored "-Wcast-align" SEXP materialize(SEXP wrapper) { - SEXP res = nullptr; - RCNTXT* cur = (RCNTXT*)R_GlobalContext; + if (auto lazyArgs = LazyArglist::check(wrapper)) { - res = lazyArgs->createArglist(); + auto res = lazyArgs->createArglist(); // Fixup the contexts chain - while (cur) { + for (auto cur = (RCNTXT*)R_GlobalContext; cur; cur = cur->nextcontext) { if (cur->promargs == wrapper) cur->promargs = res; - cur = cur->nextcontext; } - } else if (auto lazyEnv = LazyEnvironment::check(wrapper)) { - assert(!lazyEnv->materialized()); + return res; + } + if (auto lazyEnv = LazyEnvironment::check(wrapper)) { + assert(!lazyEnv->materialized()); PROTECT(wrapper); - SEXP arglist = R_NilValue; + auto arglist = R_NilValue; auto names = lazyEnv->names; for (size_t i = 0; i < lazyEnv->nargs; ++i) { - SEXP val = lazyEnv->getArg(i); + auto val = lazyEnv->getArg(i); if (val == R_UnboundValue) continue; - SEXP name = cp_pool_at(names[i]); + auto name = cp_pool_at(names[i]); if (TYPEOF(name) == LISTSXP) name = CAR(name); // cons protects its args if needed @@ -308,29 +306,29 @@ SEXP materialize(SEXP wrapper) { SET_MISSING(arglist, 2); } auto parent = lazyEnv->getParent(); - res = Rf_NewEnvironment(R_NilValue, arglist, parent); + auto res = Rf_NewEnvironment(R_NilValue, arglist, parent); + PROTECT(res); lazyEnv->materialized(res); // Make sure wrapper is not collected by the gc (we may still use it to // access the materialized env) Rf_setAttrib(res, symbol::delayedEnv, wrapper); lazyEnv->clear(); // Fixup the contexts chain - while (cur) { + for (auto cur = (RCNTXT*)R_GlobalContext; cur; cur = cur->nextcontext) { if (cur->cloenv == wrapper) cur->cloenv = res; if (cur->sysparent == wrapper) cur->sysparent = res; - cur = cur->nextcontext; } if (LazyEnvironment::check(parent)) { parent = materialize(parent); SET_ENCLOS(res, parent); } - - UNPROTECT(1); + UNPROTECT(2); + return res; } - assert(res); - return res; + + assert(false); } SEXP materializeCallerEnv(CallContext& callCtx) { @@ -1587,6 +1585,8 @@ void deoptFramesWithContext(const CallContext* callCtxt, if (auto le = LazyEnvironment::check(deoptEnv)) { assert(!le->materialized()); deoptEnv = materialize(deoptEnv); + // Still need to set the cloenv because materialize only patches the + // context list starting with R_GlobalContext cntxt->cloenv = deoptEnv; } assert(TYPEOF(deoptEnv) == ENVSXP); From 925ec4b1b359774cd747d998e18d00b64c9b871b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 14 Jun 2022 09:32:57 +0000 Subject: [PATCH 5/8] Support deopts in outermost promises This adds support for deopting from promises that are the outermost frame when deopting. The mechanism is that if the outermost frame is a promise, we don't longjump to a context (since promises don't have contexts), but rather return the result. This is the only case when deopt returns, and we immediately return also from the promise code with the result, passing it to the place where the promise forcing occured. --- rir/src/compiler/native/builtins.cpp | 55 +++++++++++++++---- rir/src/compiler/native/builtins.h | 1 + .../compiler/native/lower_function_llvm.cpp | 40 +++++++++++--- rir/src/compiler/pir/code.h | 2 + rir/src/compiler/pir/promise.h | 2 + rir/src/interpreter/interp.cpp | 36 +++++++----- 6 files changed, 101 insertions(+), 35 deletions(-) diff --git a/rir/src/compiler/native/builtins.cpp b/rir/src/compiler/native/builtins.cpp index a6497c76e..548fe4c47 100644 --- a/rir/src/compiler/native/builtins.cpp +++ b/rir/src/compiler/native/builtins.cpp @@ -828,8 +828,8 @@ static SEXP deoptSentinelContainer = []() { return store; }(); -void deoptImpl(rir::Code* c, SEXP cls, DeoptMetadata* m, R_bcstack_t* args, - bool leakedEnv, DeoptReason* deoptReason, SEXP deoptTrigger) { +SEXP deopt(rir::Code* c, SEXP cls, DeoptMetadata* m, R_bcstack_t* args, + bool leakedEnv, DeoptReason* deoptReason, SEXP deoptTrigger) { deoptReason->record(deoptTrigger); assert(m->numFrames >= 1); @@ -854,8 +854,9 @@ void deoptImpl(rir::Code* c, SEXP cls, DeoptMetadata* m, R_bcstack_t* args, auto le = LazyEnvironment::check(env); if (deoptless && m->numFrames == 1 && cls != deoptlessRecursion && ((le && !le->materialized()) || - (!le && (!leakedEnv || !deoptlessNoLeakedEnvs)))) { - assert(m->frames[0].inPromise == false); + (!le && (!leakedEnv || !deoptlessNoLeakedEnvs))) && + /* TODO: support deoptless when outermost frame is a promise */ + !m->frames[0].inPromise) { size_t envSize = le ? le->nargs : Rf_length(FRAME(env)); if (envSize <= DeoptContext::MAX_ENV && @@ -932,8 +933,8 @@ void deoptImpl(rir::Code* c, SEXP cls, DeoptMetadata* m, R_bcstack_t* args, Rf_findcontext(CTXT_BROWSER | CTXT_FUNCTION, originalCntxt->cloenv, res); - assert(false); - return; + assert(false && "unreachable after deoptless"); + return nullptr; } } } @@ -945,13 +946,35 @@ void deoptImpl(rir::Code* c, SEXP cls, DeoptMetadata* m, R_bcstack_t* args, if (f->body() == c) Pool::patch(idx, deoptSentinelContainer); - CallContext call(ArglistOrder::NOT_REORDERED, c, cls, - /* nargs */ -1, src_pool_at(c->src), args, - (Immediate*)nullptr, env, R_NilValue, Context()); + if (cls) { + CallContext call(ArglistOrder::NOT_REORDERED, c, cls, + /* nargs */ -1, src_pool_at(c->src), args, + (Immediate*)nullptr, env, R_NilValue, Context()); - deoptFramesWithContext(&call, m, R_NilValue, m->numFrames - 1, stackHeight, - (RCNTXT*)R_GlobalContext); - assert(false); + // Deopt in a function longjumps to its context + deoptFramesWithContext(&call, m, R_NilValue, m->numFrames - 1, + stackHeight, (RCNTXT*)R_GlobalContext); + assert(false && "unreachable after deopt"); + return nullptr; + } else { + // Deopt in a promise has nowhere to longjump, so it leaves the result + // on the TOS and returns here, this is immediately returned as the + // result of the promise + deoptFramesWithContext(nullptr, m, R_NilValue, m->numFrames - 1, + stackHeight, (RCNTXT*)R_GlobalContext); + return ostack_pop(); + } +} + +void deoptImpl(rir::Code* c, SEXP cls, DeoptMetadata* m, R_bcstack_t* args, + bool leakedEnv, DeoptReason* deoptReason, SEXP deoptTrigger) { + deopt(c, cls, m, args, leakedEnv, deoptReason, deoptTrigger); +} + +SEXP deoptPromImpl(rir::Code* c, DeoptMetadata* m, R_bcstack_t* args, + bool leakedEnv, DeoptReason* deoptReason, + SEXP deoptTrigger) { + return deopt(c, nullptr, m, args, leakedEnv, deoptReason, deoptTrigger); } void recordTypefeedbackImpl(Opcode* pos, rir::Code* code, SEXP value) { @@ -2436,6 +2459,14 @@ void NativeBuiltins::initializeBuiltins() { t::DeoptReasonPtr, t::SEXP}, false), {llvm::Attribute::NoReturn}}; + get_(Id::deoptProm) = { + "deoptProm", + (void*)&deoptPromImpl, + llvm::FunctionType::get(t::SEXP, + {t::voidPtr, t::voidPtr, t::stackCellPtr, t::i1, + t::DeoptReasonPtr, t::SEXP}, + false), + {}}; get_(Id::assertFail) = {"assertFail", (void*)&assertFailImpl, t::void_voidPtr, diff --git a/rir/src/compiler/native/builtins.h b/rir/src/compiler/native/builtins.h index dfaf0dadc..0ac066f88 100644 --- a/rir/src/compiler/native/builtins.h +++ b/rir/src/compiler/native/builtins.h @@ -87,6 +87,7 @@ struct NativeBuiltins { length, recordTypefeedback, deopt, + deoptProm, assertFail, printValue, extract11, diff --git a/rir/src/compiler/native/lower_function_llvm.cpp b/rir/src/compiler/native/lower_function_llvm.cpp index c315ae1fc..b99dc8e9d 100644 --- a/rir/src/compiler/native/lower_function_llvm.cpp +++ b/rir/src/compiler/native/lower_function_llvm.cpp @@ -3583,15 +3583,37 @@ void LowerFunctionLLVM::compile() { target->addExtraPoolEntry(store); } - withCallFrame(args, [&]() { - return call(NativeBuiltins::get(NativeBuiltins::Id::deopt), - {paramCode(), paramClosure(), - convertToPointer(m, t::i8, true), paramArgs(), - c(deopt->escapedEnv, 1), - load(deopt->deoptReason()), - loadSxp(deopt->deoptTrigger())}); - }); - builder.CreateUnreachable(); + // Deopt only returns if the outermost frame is a promise. + // In that case, the result is the returned value and we simply + // return it from here. + if (code->isPromise()) { + auto res = withCallFrame( + args, + [&]() { + return call(NativeBuiltins::get( + NativeBuiltins::Id::deoptProm), + {paramCode(), + convertToPointer(m, t::i8, true), + paramArgs(), c(deopt->escapedEnv, 1), + load(deopt->deoptReason()), + loadSxp(deopt->deoptTrigger())}); + }, + false); + exitBlocks.push_back(builder.GetInsertBlock()); + builder.CreateRet(res); + } else { + withCallFrame(args, [&]() { + return call( + NativeBuiltins::get(NativeBuiltins::Id::deopt), + {paramCode(), paramClosure(), + convertToPointer(m, t::i8, true), paramArgs(), + c(deopt->escapedEnv, 1), + load(deopt->deoptReason()), + loadSxp(deopt->deoptTrigger())}); + }); + insn_assert(builder.getFalse(), "unreachable after deopt"); + builder.CreateUnreachable(); + } break; } diff --git a/rir/src/compiler/pir/code.h b/rir/src/compiler/pir/code.h index 31be17e24..c89c5df22 100644 --- a/rir/src/compiler/pir/code.h +++ b/rir/src/compiler/pir/code.h @@ -34,6 +34,8 @@ class Code { virtual rir::Code* rirSrc() const = 0; virtual void printName(std::ostream&) const = 0; + virtual bool isPromise() const { return false; } + friend std::ostream& operator<<(std::ostream& out, const Code& e) { e.printName(out); return out; diff --git a/rir/src/compiler/pir/promise.h b/rir/src/compiler/pir/promise.h index ba829892b..52bfb70d4 100644 --- a/rir/src/compiler/pir/promise.h +++ b/rir/src/compiler/pir/promise.h @@ -22,6 +22,8 @@ class Promise : public Code { void printName(std::ostream& out) const override; + bool isPromise() const override final { return true; } + private: rir::Code* rirSrc_; const unsigned srcPoolIdx_; diff --git a/rir/src/interpreter/interp.cpp b/rir/src/interpreter/interp.cpp index c081294e0..d8b10df08 100644 --- a/rir/src/interpreter/interp.cpp +++ b/rir/src/interpreter/interp.cpp @@ -1655,23 +1655,31 @@ void deoptFramesWithContext(const CallContext* callCtxt, SEXP res = trampoline(); assert(ostack_length() == frameBaseSize); - if (!outermostFrame) { - if (!inPromise) - endClosureContext(cntxt, res); - } else { - // Deopt in promise is only possible if the promise is not the outermost - // frame. The reason is that the promise does not have a context - // associated and we can therefore not jump out of deoptimized promise - // evaluation. Therefore promises should only occur in inner frames - // during deoptimization. - assert(!inPromise); + if (outermostFrame) { endDeoptimizing(); - assert(findFunctionContextFor(deoptEnv) == cntxt); - // long-jump out of all the inlined contexts - Rf_findcontext(CTXT_BROWSER | CTXT_FUNCTION, cntxt->cloenv, res); - assert(false); + + if (!inPromise) { + assert(findFunctionContextFor(deoptEnv) == cntxt); + // long-jump out of all the inlined contexts + Rf_findcontext(CTXT_BROWSER | CTXT_FUNCTION, cntxt->cloenv, res); + assert(false && "unreachable"); + } else { + // Deopt in a promise that is the outermost frame: + // The promise does not have a context associated and we can + // therefore not jump out of deoptimized promise evaluation. In this + // case we leave the result on the top of the stack and the native + // backend knows that if the deopt returns, it should pop the result + // from the stack and return it as the promise's result. + + // assert(false); + } } + assert(!outermostFrame || inPromise); + + if (!inPromise) + endClosureContext(cntxt, res); + ostack_push(res); } From 90e0880b582e80d2dc2c4de87df205b95b24f460 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 14 Jun 2022 09:38:50 +0000 Subject: [PATCH 6/8] don't inline a promise if the force doesn't have a framestate --- rir/src/compiler/opt/force_dominance.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/rir/src/compiler/opt/force_dominance.cpp b/rir/src/compiler/opt/force_dominance.cpp index dcc0d4e9d..fe91e4dcc 100644 --- a/rir/src/compiler/opt/force_dominance.cpp +++ b/rir/src/compiler/opt/force_dominance.cpp @@ -192,7 +192,14 @@ bool ForceDominance::apply(Compiler&, ClosureVersion* cls, Code* code, Value* eager = mkarg->eagerArg(); f->replaceUsesWith(eager); next = bb->remove(ip); - } else if (toInline.count(f)) { + } else if (toInline.count(f) && + // Can't inline a promise with Assumes if the + // dominating Force doesn't have a Framestate + (f->frameState() || + Visitor::check(mkarg->prom()->entry, + [](Instruction* i) { + return !Assume::Cast(i); + }))) { anyChange = true; Promise* prom = mkarg->prom(); BB* split = From c572a49839cc5e6a3ab2e5831d06ca373d007dce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Wed, 15 Jun 2022 09:49:31 +0000 Subject: [PATCH 7/8] speculate in promises (except when inlining them) --- rir/src/compiler/analysis/verifier.cpp | 23 ++----- rir/src/compiler/compiler.cpp | 2 +- rir/src/compiler/opt/pass_definitions.h | 2 - rir/src/compiler/rir2pir/rir2pir.cpp | 79 +++++++++++++------------ rir/src/compiler/rir2pir/rir2pir.h | 2 +- rir/src/interpreter/interp.cpp | 2 - 6 files changed, 48 insertions(+), 62 deletions(-) diff --git a/rir/src/compiler/analysis/verifier.cpp b/rir/src/compiler/analysis/verifier.cpp index 4cedab71d..55881bfef 100644 --- a/rir/src/compiler/analysis/verifier.cpp +++ b/rir/src/compiler/analysis/verifier.cpp @@ -35,7 +35,7 @@ class TheVerifier { std::unordered_set seenPreds; void operator()() { - Visitor::run(f->entry, [&](BB* bb) { return verify(bb, false); }); + Visitor::run(f->entry, [&](BB* bb) { return verify(bb); }); Visitor::run(f->entry, [&](BB* bb) { seenPreds.erase(bb); }); if (!seenPreds.empty()) { std::cerr << "The following preds are not reachable from entry: "; @@ -98,7 +98,7 @@ class TheVerifier { return doms.at(c); } - void verify(BB* bb, bool inPromise) { + void verify(BB* bb) { if (bb->id >= bb->owner->nextBBId) { std::cout << "BB" << bb->id << " id is bigger than max (" << bb->owner->nextBBId << ")\n"; @@ -121,7 +121,7 @@ class TheVerifier { } for (auto i : *bb) { - verify(i, bb, inPromise); + verify(i, bb); } /* This check verifies that our graph is in edge-split format. Currently we do not rely on this property, however we should @@ -196,10 +196,10 @@ class TheVerifier { } void verify(Promise* p) { - Visitor::run(p->entry, [&](BB* bb) { verify(bb, true); }); + Visitor::run(p->entry, [&](BB* bb) { verify(bb); }); } - void verify(Instruction* i, BB* bb, bool inPromise) { + void verify(Instruction* i, BB* bb) { if (i->bb() != bb) { std::cerr << "Error: instruction '"; i->print(std::cerr); @@ -269,19 +269,6 @@ class TheVerifier { }); } - if (i->frameState()) { - if (!inPromise) { - auto fs = i->frameState(); - while (fs->next()) - fs = fs->next(); - if (fs->inPromise) { - std::cerr << "Error: instruction '"; - i->print(std::cerr); - std::cerr << "' outermost fs inPromis in body code\n"; - ok = false; - } - } - } if (auto assume = Assume::Cast(i)) { if (IsType::Cast(assume->arg(0).val())) { if (!assume->reason.pc()) { diff --git a/rir/src/compiler/compiler.cpp b/rir/src/compiler/compiler.cpp index b4c367e28..c432f564b 100644 --- a/rir/src/compiler/compiler.cpp +++ b/rir/src/compiler/compiler.cpp @@ -155,7 +155,7 @@ void Compiler::compileClosure(Closure* closure, rir::Function* optFunction, auto arg = closure->formals().defaultArgs()[idx]; assert(rir::Code::check(arg) && "Default arg not compiled"); auto code = rir::Code::unpack(arg); - auto res = rir2pir.tryCreateArg(code, builder, false); + auto res = rir2pir.tryCreateArg(code, builder); if (!res) { failedToCompileDefaultArgs = true; return; diff --git a/rir/src/compiler/opt/pass_definitions.h b/rir/src/compiler/opt/pass_definitions.h index 37cb3bb21..b8634ceac 100644 --- a/rir/src/compiler/opt/pass_definitions.h +++ b/rir/src/compiler/opt/pass_definitions.h @@ -71,7 +71,6 @@ PASS(DelayEnv, false, false) * passes will do the smart parts. */ PASS(Inline, false, false) -// PASS(Inline, true, false) /* * Goes through every operation that for the general case needs an environment @@ -145,7 +144,6 @@ PASS(LoadElision, false, false) PASS(TypeInference, true, false) PASS(TypeSpeculation, false, false) -// PASS(TypeSpeculation, true, false) PASS(PromiseSplitter, false, false) diff --git a/rir/src/compiler/rir2pir/rir2pir.cpp b/rir/src/compiler/rir2pir/rir2pir.cpp index f3f569a74..2840250fb 100644 --- a/rir/src/compiler/rir2pir/rir2pir.cpp +++ b/rir/src/compiler/rir2pir/rir2pir.cpp @@ -163,8 +163,7 @@ Checkpoint* Rir2Pir::addCheckpoint(rir::Code* srcCode, Opcode* pos, return insert.emitCheckpoint(srcCode, pos, stack, inPromise()); } -Value* Rir2Pir::tryCreateArg(rir::Code* promiseCode, Builder& insert, - bool eager) { +Value* Rir2Pir::tryCreateArg(rir::Code* promiseCode, Builder& insert) { Promise* prom = insert.function->createProm(promiseCode); { Builder promiseBuilder(insert.function, prom); @@ -175,7 +174,7 @@ Value* Rir2Pir::tryCreateArg(rir::Code* promiseCode, Builder& insert, } Value* eagerVal = UnboundValue::instance(); - if (eager || Query::pureExceptDeopt(prom)) { + if (Query::pureExceptDeopt(prom)) { eagerVal = tryInlinePromise(promiseCode, insert); if (!eagerVal) { log.warn("Failed to inline a promise"); @@ -183,11 +182,6 @@ Value* Rir2Pir::tryCreateArg(rir::Code* promiseCode, Builder& insert, } } - if (eager) { - assert(eagerVal != UnboundValue::instance()); - return eagerVal; - } - return insert(new MkArg(prom, eagerVal, insert.env)); } @@ -437,7 +431,7 @@ bool Rir2Pir::compileBC(const BC& bc, Opcode* pos, Opcode* nextPos, // If this call was never executed we might as well compile an // unconditional deopt. - if (!inPromise() && !inlining() && feedback.taken == 0 && + if (!inlining() && feedback.taken == 0 && insert.function->optFunction->invocationCount() > 1 && srcCode->function()->deadCallReached() < 3) { auto sp = @@ -669,8 +663,8 @@ bool Rir2Pir::compileBC(const BC& bc, Opcode* pos, Opcode* nextPos, // Insert a guard if we want to speculate if (!staticMonomorphicBuiltin && (monomorphicBuiltin || monomorphicClosure || monomorphicSpecial)) { - // Can't speculate in promises - if (inPromise()) { + // Can't speculate while inlining + if (inlining()) { monomorphicBuiltin = monomorphicClosure = monomorphicSpecial = false; } else { @@ -683,12 +677,12 @@ bool Rir2Pir::compileBC(const BC& bc, Opcode* pos, Opcode* nextPos, auto bb = cp->nextBB(); auto dummyPos = bb->begin(); - Value* expection = nullptr; + Value* expectation = nullptr; if (ldfun && localFuns.count(ldfun->varName)) { auto mk = localFuns.at(ldfun->varName); if (mk && mk->originalBody->container() == BODY(ti.monomorphic)) { - expection = mk; + expectation = mk; // Even though we statically know the env, we must // compile an Env::unclosed() closure here, since we // cannot pass the pir env from the host function to @@ -701,37 +695,46 @@ bool Rir2Pir::compileBC(const BC& bc, Opcode* pos, Opcode* nextPos, guardedCallee = BBTransform::insertCalleeGuard( compiler, ti, DeoptReason(ti.feedbackOrigin, DeoptReason::CallTarget), - callee, stableEnv || expection, expection, cp, bb, + callee, stableEnv || expectation, expectation, cp, bb, dummyPos); } } } auto eagerEval = [&](Value*& arg, size_t i, bool promiseWrapped) { - if (auto mk = MkArg::Cast(arg)) { - if (mk->isEager()) { - arg = mk->eagerArg(); - } else { - auto original = arg; - arg = tryCreateArg(mk->prom()->rirSrc(), insert, true); - if (promiseWrapped) - arg = insert(new MkArg(mk->prom(), arg, mk->env())); - if (!arg) { - log.warn("Failed to compile a promise"); - return false; - } - if (i != (size_t)-1 && at(nargs - 1 - i) == original) { - // Inlined argument evaluation might have side effects. - // Let's have a checkpoint here. This checkpoint needs - // to capture the so far evaluated promises. - stack.at(nargs - 1 - i) = - promiseWrapped - ? arg - : insert(new MkArg(mk->prom(), arg, mk->env())); - addCheckpoint(srcCode, pos, stack, insert); - } - } + if (!MkArg::Cast(arg)) { + return true; + } + + auto mk = MkArg::Cast(arg); + if (mk->isEager()) { + arg = mk->eagerArg(); + return true; } + + assert(!inlining()); + auto original = arg; + arg = tryInlinePromise(mk->prom()->rirSrc(), insert); + if (!arg) { + log.warn("Failed to inline a promise"); + return false; + } + + if (promiseWrapped) { + arg = insert(new MkArg(mk->prom(), arg, mk->env())); + } + + if (i != (size_t)-1 && at(nargs - 1 - i) == original) { + // Inlined argument evaluation might have side effects. + // Let's have a checkpoint here. This checkpoint needs + // to capture the so far evaluated promises. + stack.at(nargs - 1 - i) = + promiseWrapped + ? arg + : insert(new MkArg(mk->prom(), arg, mk->env())); + addCheckpoint(srcCode, pos, stack, insert); + } + return true; }; @@ -1453,7 +1456,7 @@ Value* Rir2Pir::tryTranslate(rir::Code* srcCode, Builder& insert, Opcode* start, auto asBool = insert( new Identical(branchCondition, branchReason, PirType::val())); - if (!inPromise()) { + if (!inlining()) { if (auto c = Instruction::Cast(branchCondition)) { auto likely = c->typeFeedback().value; if (likely == True::instance() || diff --git a/rir/src/compiler/rir2pir/rir2pir.h b/rir/src/compiler/rir2pir/rir2pir.h index afe022161..fef4ab41b 100644 --- a/rir/src/compiler/rir2pir/rir2pir.h +++ b/rir/src/compiler/rir2pir/rir2pir.h @@ -24,7 +24,7 @@ class Rir2Pir { const std::vector& initialStack) __attribute__((warn_unused_result)); - Value* tryCreateArg(rir::Code* prom, Builder& insert, bool eager) + Value* tryCreateArg(rir::Code* prom, Builder& insert) __attribute__((warn_unused_result)); typedef std::unordered_map CallTargetCheckpoints; diff --git a/rir/src/interpreter/interp.cpp b/rir/src/interpreter/interp.cpp index d8b10df08..06288f680 100644 --- a/rir/src/interpreter/interp.cpp +++ b/rir/src/interpreter/interp.cpp @@ -1670,8 +1670,6 @@ void deoptFramesWithContext(const CallContext* callCtxt, // case we leave the result on the top of the stack and the native // backend knows that if the deopt returns, it should pop the result // from the stack and return it as the promise's result. - - // assert(false); } } From 36de7635100c6397c35795cccbfa6e6c50ea0750 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Thu, 16 Jun 2022 11:58:44 +0000 Subject: [PATCH 8/8] fix --- rir/src/compiler/pir/instruction.h | 16 ++++------------ rir/src/compiler/rir2pir/rir2pir.h | 4 ++++ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/rir/src/compiler/pir/instruction.h b/rir/src/compiler/pir/instruction.h index f186cca50..4774a5f51 100644 --- a/rir/src/compiler/pir/instruction.h +++ b/rir/src/compiler/pir/instruction.h @@ -2363,9 +2363,6 @@ class VLIE(CallBuiltin, Effects::Any()), public CallInstruction { Value* callerEnv() { return env(); } VisibilityFlag visibilityFlag() const override; - Value* frameStateOrTs() const override final { - return Tombstone::framestate(); - } private: CallBuiltin(Value * callerEnv, SEXP builtin, @@ -2406,9 +2403,6 @@ class VLI(CallSafeBuiltin, Effects(Effect::Warn) | Effect::Error | const override final; VisibilityFlag visibilityFlag() const override; - Value* frameStateOrTs() const override final { - return Tombstone::framestate(); - } size_t gvnBase() const override; }; @@ -2668,9 +2662,9 @@ class VLI(Phi, Effects::None()) { // Instructions targeted specially for speculative optimization /* - * Must be the last instruction of a BB with two childs. One should + * Must be the last instruction of a BB with two children. One should * contain a deopt. Checkpoint takes either branch at random - * to ensure the optimizer consider deopt and non-deopt cases. + * to ensure the optimizer considers both deopt and non-deopt cases. */ class Checkpoint : public FixedLenInstruction().val(); } - FrameState* frameState() const { - return FrameState::Cast(frameStateOrTs()); - } + void updateFrameState(Value* fs) override final { arg<0>().val() = fs; } bool hasDeoptReason() const; diff --git a/rir/src/compiler/rir2pir/rir2pir.h b/rir/src/compiler/rir2pir/rir2pir.h index fef4ab41b..ea19aff21 100644 --- a/rir/src/compiler/rir2pir/rir2pir.h +++ b/rir/src/compiler/rir2pir/rir2pir.h @@ -92,6 +92,10 @@ class PromiseRir2Pir : public Rir2Pir { : Rir2Pir(cmp, cls, log, name, outerFeedback), inlining_(inlining) {} private: + // This is used for disabling speculation while inlining a promise in + // rir2pir (eg, eager eval for builtins). The problem is that the inlined + // promise doesn't have a location in rir bytecode, and so deopts would have + // nowhere to jump... bool inlining_; bool inlining() const override final { return inlining_; } bool inPromise() const override final { return true; }