From b670cb7fd745b891cb1e109cd6f389e659ec15f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 26 Jul 2022 17:36:01 +0000 Subject: [PATCH 1/5] Fix verifier choking when inlining a promise in the RIR bytecode compiler `function() invisible(return(5))` would trip the bytecode verifier since we'd emit ret_ in the inlined promise in the middle of the `invisible` call sequence, leaving stuff on the stack... For now, track when inlining promises and emit return_ in those cases (since return_ does a longjump and resets stack). Ideally we would just not emit any code after finding a call to return, but this would be more involved to change. --- rir/src/bc/Compiler.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/rir/src/bc/Compiler.cpp b/rir/src/bc/Compiler.cpp index 2ad133d76..667915acf 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) {} @@ -144,6 +145,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() { @@ -1097,7 +1101,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(); @@ -1748,7 +1752,9 @@ 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; } @@ -1776,7 +1782,9 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, 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 From 32ba6b5c4251151c08ab074ee64305b2483f7950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 26 Jul 2022 17:42:17 +0000 Subject: [PATCH 2/5] cleanups --- rir/src/bc/CodeVerifier.cpp | 3 +-- rir/src/bc/Compiler.cpp | 10 ++++++---- 2 files changed, 7 insertions(+), 6 deletions(-) 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 667915acf..fac838afc 100644 --- a/rir/src/bc/Compiler.cpp +++ b/rir/src/bc/Compiler.cpp @@ -116,7 +116,6 @@ class CompilerContext { }; class PromiseContext : public CodeContext { - public: PromiseContext(SEXP ast, FunctionWriter& fun, CodeContext* p) : CodeContext(ast, fun, p) {} @@ -1759,7 +1758,7 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, } // 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: @@ -1777,6 +1776,7 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, cs << BC::push(eager); return; } + } Code* prom; if (arg_type == ArgType::EAGER_PROMISE) { @@ -1788,12 +1788,14 @@ static void compileLoadOneArg(CompilerContext& ctx, SEXP arg, ArgType arg_type, 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); From 6273c2b5a540340e81a35563fa15e550f7d1cf6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 26 Jul 2022 17:44:56 +0000 Subject: [PATCH 3/5] compile a little cleaner (cond, true [, false]) --- rir/src/bc/Compiler.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/rir/src/bc/Compiler.cpp b/rir/src/bc/Compiler.cpp index fac838afc..037587471 100644 --- a/rir/src/bc/Compiler.cpp +++ b/rir/src/bc/Compiler.cpp @@ -1055,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; } From b681cb9c9816d640d952047c8c8be03b3c359164 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 26 Jul 2022 17:46:01 +0000 Subject: [PATCH 4/5] inline invisible calls in bc --- rir/src/R/symbol_list.h | 1 + rir/src/bc/Compiler.cpp | 15 +++++++++++++++ 2 files changed, 16 insertions(+) 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/Compiler.cpp b/rir/src/bc/Compiler.cpp index 037587471..9b07ce120 100644 --- a/rir/src/bc/Compiler.cpp +++ b/rir/src/bc/Compiler.cpp @@ -1097,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); From 1692a762abae015e83fc19eb1292a98ef348e655 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Je=C4=8Dmen?= Date: Tue, 26 Jul 2022 17:46:49 +0000 Subject: [PATCH 5/5] CodeStream checks if the inserted bc will be unreachable --- rir/src/bc/CodeStream.h | 16 ++++++++++++++++ rir/src/bc/Compiler.cpp | 10 ++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) 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/Compiler.cpp b/rir/src/bc/Compiler.cpp index 9b07ce120..1b7c640ce 100644 --- a/rir/src/bc/Compiler.cpp +++ b/rir/src/bc/Compiler.cpp @@ -1389,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; } } @@ -1404,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; } } @@ -2026,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(); } @@ -2080,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());