Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bc if invisible #1232

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+73 −25
Open

Bc if invisible #1232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b670cb7
Fix verifier choking when inlining a promise in the RIR bytecode comp…
JanJecmen Jul 26, 2022
32ba6b5
cleanups
JanJecmen Jul 26, 2022
6273c2b
compile a little cleaner (cond, true [, false])
JanJecmen Jul 26, 2022
b681cb9
inline invisible calls in bc
JanJecmen Jul 26, 2022
1692a76
CodeStream checks if the inserted bc will be unreachable
JanJecmen Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rir/src/R/symbol_list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand Down
16 changes: 16 additions & 0 deletions rir/src/bc/CodeStream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -80,14 +86,22 @@ 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);
return *this;
}

CodeStream& operator<<(BC::Label label) {
nextPosUnreachable = false;

// get rid of unnecessary jumps
{
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions rir/src/bc/CodeVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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<Immediate*>(cptr + 1);
Expand Down
78 changes: 55 additions & 23 deletions rir/src/bc/Compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class CompilerContext {
std::stack<LoopContext> loops;
CodeContext* parent;
std::unordered_map<SEXP, CacheSlotNumber> loadsSlotInCache;
bool inliningPromise = false;

CodeContext(SEXP ast, FunctionWriter& fun, CodeContext* p)
: cs(fun, ast), parent(p) {}
Expand Down Expand Up @@ -115,7 +116,6 @@ class CompilerContext {
};

class PromiseContext : public CodeContext {

public:
PromiseContext(SEXP ast, FunctionWriter& fun, CodeContext* p)
: CodeContext(ast, fun, p) {}
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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;
}

Expand All @@ -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);

Expand All @@ -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();
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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:
Expand All @@ -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);
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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());

Expand Down