diff --git a/rir/src/R/symbol_list.h b/rir/src/R/symbol_list.h index 07a43708c..6293a4dc3 100644 --- a/rir/src/R/symbol_list.h +++ b/rir/src/R/symbol_list.h @@ -56,6 +56,7 @@ V(ispairlist, "is.pairlist") \ V(quote, "quote") \ V(Missing, "missing") \ + V(Invisible, "invisible") \ V(seq, "seq") \ V(lapply, "lapply") \ V(aslist, "as.list") \ diff --git a/rir/src/bc/CodeStream.h b/rir/src/bc/CodeStream.h index b84dd9b81..db259224a 100644 --- a/rir/src/bc/CodeStream.h +++ b/rir/src/bc/CodeStream.h @@ -24,6 +24,12 @@ class CodeStream { unsigned size = 1024; unsigned nops = 0; + // Flag that tracks if the next inserted BC will be dead code. The first BC + // is always reachable since it's the entrypoint. Inserting an exit or + // uncontidional jump makes the next position unreachable. Inserting a label + // makes it reachable again since it's a possible jump target. + bool nextPosUnreachable = false; + FunctionWriter& function; Preserve preserve; @@ -80,7 +86,14 @@ class CodeStream { : needed + align - (needed % align); } + bool isNextPosUnreachable() const { return nextPosUnreachable; } + CodeStream& operator<<(const BC& b) { + SLOWASSERT(!nextPosUnreachable && + "inserting dead code into CodeStream"); + // TODO: should be `b.isExit()` but when we inline a promise that has a + // return_ we still emit the rest of the sequence that uses it... + nextPosUnreachable = b.bc == Opcode::ret_ || b.isUncondJmp(); if (b.bc == Opcode::nop_) nops++; b.write(*this); @@ -88,6 +101,7 @@ class CodeStream { } CodeStream& operator<<(BC::Label label) { + nextPosUnreachable = false; // get rid of unnecessary jumps { @@ -144,6 +158,8 @@ class CodeStream { } Code* finalize(size_t localsCnt, size_t bindingsCnt) { + SLOWASSERT(nextPosUnreachable && "CodeStream should end with a BC that " + "is an exit or an unconditional jump"); Code* res = function.writeCode(ast, &(*code)[0], pos, sources, patchpoints, labels, localsCnt, nops, bindingsCnt); diff --git a/rir/src/bc/CodeVerifier.cpp b/rir/src/bc/CodeVerifier.cpp index 102068d82..66c40acdb 100644 --- a/rir/src/bc/CodeVerifier.cpp +++ b/rir/src/bc/CodeVerifier.cpp @@ -123,7 +123,7 @@ void CodeVerifier::calculateAndVerifyStack(Code* code) { Opcode* cptr = code->code(); q.push(State(cptr)); - while (not q.empty()) { + while (!q.empty()) { State i = q.top(); q.pop(); if (state.find(i.pc) != state.end()) { @@ -259,7 +259,6 @@ void CodeVerifier::verifyFunctionLayout(SEXP sexp) { Rf_error("RIR Verifier: cached clear_binding_cache_ with " "invalid index"); } - if (*cptr == Opcode::mk_promise_ || *cptr == Opcode::mk_eager_promise_) { unsigned* promidx = reinterpret_cast(cptr + 1); diff --git a/rir/src/bc/Compiler.cpp b/rir/src/bc/Compiler.cpp index 2ad133d76..1b7c640ce 100644 --- a/rir/src/bc/Compiler.cpp +++ b/rir/src/bc/Compiler.cpp @@ -76,6 +76,7 @@ class CompilerContext { std::stack loops; CodeContext* parent; std::unordered_map loadsSlotInCache; + bool inliningPromise = false; CodeContext(SEXP ast, FunctionWriter& fun, CodeContext* p) : cs(fun, ast), parent(p) {} @@ -115,7 +116,6 @@ class CompilerContext { }; class PromiseContext : public CodeContext { - public: PromiseContext(SEXP ast, FunctionWriter& fun, CodeContext* p) : CodeContext(ast, fun, p) {} @@ -144,6 +144,9 @@ class CompilerContext { bool inLoop() const { return code.top()->inLoop(); } + bool inliningPromise() const { return code.top()->inliningPromise; } + void setInliningPromise(bool val) { code.top()->inliningPromise = val; } + LoopContext& loop() { return code.top()->loops.top(); } bool loopNeedsContext() { @@ -1052,26 +1055,31 @@ bool compileSpecialCall(CompilerContext& ctx, SEXP ast, SEXP fun, SEXP args_, return false; emitGuardForNamePrimitive(cs, fun); - BC::Label trueBranch = cs.mkLabel(); - BC::Label nextBranch = cs.mkLabel(); + BC::Label falseBranch = cs.mkLabel(); compileExpr(ctx, args[0]); - cs << BC::asbool() << BC::brtrue(trueBranch); + cs << BC::asbool() << BC::brfalse(falseBranch); - if (args.length() < 3) { - if (!voidContext) { - cs << BC::push(R_NilValue); - cs << BC::invisible(); - } + compileExpr(ctx, args[1], voidContext); + + if (args.length() == 2 && voidContext) { + // No else branch needed + cs << falseBranch; } else { - compileExpr(ctx, args[2], voidContext); - } - cs << BC::br(nextBranch); + BC::Label nextBranch = cs.mkLabel(); + cs << BC::br(nextBranch) << falseBranch; + + if (args.length() == 3) { + // There's an else branch in the code + compileExpr(ctx, args[2], voidContext); + } else if (!voidContext) { + // No else branch in the code but still need the NULL + cs << BC::push(R_NilValue) << BC::invisible(); + } - cs << trueBranch; - compileExpr(ctx, args[1], voidContext); + cs << nextBranch; + } - cs << nextBranch; return true; } @@ -1089,6 +1097,21 @@ bool compileSpecialCall(CompilerContext& ctx, SEXP ast, SEXP fun, SEXP args_, return true; } + if (fun == symbol::Invisible && args.length() < 2) { + emitGuardForNamePrimitive(cs, fun); + + if (args.length() == 0) { + cs << BC::push(R_NilValue); + } else { + ctx.setInliningPromise(true); + compileExpr(ctx, args[0]); + ctx.setInliningPromise(false); + } + + cs << BC::invisible(); + return true; + } + if (fun == symbol::Return && args.length() < 2) { emitGuardForNamePrimitive(cs, fun); @@ -1097,7 +1120,7 @@ bool compileSpecialCall(CompilerContext& ctx, SEXP ast, SEXP fun, SEXP args_, else compileExpr(ctx, args[0]); - if (ctx.inLoop() || ctx.isInPromise()) + if (ctx.inLoop() || ctx.isInPromise() || ctx.inliningPromise()) cs << BC::return_(); else cs << BC::ret(); @@ -1366,7 +1389,7 @@ bool compileSpecialCall(CompilerContext& ctx, SEXP ast, SEXP fun, SEXP args_, if (ctx.loopIsLocal()) { emitGuardForNamePrimitive(cs, fun); - cs << BC::br(ctx.loopNext()) << BC::push(R_NilValue); + cs << BC::br(ctx.loopNext()); return true; } } @@ -1381,7 +1404,7 @@ bool compileSpecialCall(CompilerContext& ctx, SEXP ast, SEXP fun, SEXP args_, if (ctx.loopIsLocal()) { emitGuardForNamePrimitive(cs, fun); - cs << BC::br(ctx.loopBreak()) << BC::push(R_NilValue); + cs << BC::br(ctx.loopBreak()); return true; } } @@ -1748,12 +1771,14 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, } if (arg_type == ArgType::RAW_VALUE) { + ctx.setInliningPromise(true); compileExpr(ctx, CAR(arg), false); + ctx.setInliningPromise(false); return; } // Constant arguments do not need to be promise wrapped - if (arg_type != ArgType::EAGER_PROMISE_FROM_TOS) + if (arg_type != ArgType::EAGER_PROMISE_FROM_TOS) { switch (TYPEOF(CAR(arg))) { case LANGSXP: case SYMSXP: @@ -1771,21 +1796,26 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, cs << BC::push(eager); return; } + } Code* prom; if (arg_type == ArgType::EAGER_PROMISE) { // Compile the expression to evaluate it eagerly, and // wrap the return value in a promise without rir code + ctx.setInliningPromise(true); compileExpr(ctx, CAR(arg), false); + ctx.setInliningPromise(false); prom = compilePromiseNoRir(ctx, CAR(arg)); } else if (arg_type == ArgType::EAGER_PROMISE_FROM_TOS) { // The value we want to wrap in the argument's promise is - // already on TOS, no nead to compile the expression. + // already on TOS, no need to compile the expression. // Wrap it in a promise without rir code. prom = compilePromiseNoRir(ctx, CAR(arg)); - } else { // ArgType::PROMISE + } else if (arg_type == ArgType::PROMISE) { // Compile the expression as a promise. prom = compilePromise(ctx, CAR(arg)); + } else { + assert(false); } size_t idx = cs.addPromise(prom); @@ -1996,7 +2026,8 @@ void compileExpr(CompilerContext& ctx, SEXP exp, bool voidContext) { Code* compilePromise(CompilerContext& ctx, SEXP exp) { ctx.pushPromiseContext(exp); compileExpr(ctx, exp); - ctx.cs() << BC::ret(); + if (!ctx.cs().isNextPosUnreachable()) + ctx.cs() << BC::ret(); return ctx.pop(); } @@ -2050,7 +2081,8 @@ SEXP Compiler::finalize() { scanNames(exp); compileExpr(ctx, exp); - ctx.cs() << BC::ret(); + if (!ctx.cs().isNextPosUnreachable()) + ctx.cs() << BC::ret(); Code* body = ctx.pop(); function.finalize(body, signature, Context());