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

Fix rir inlining promises #1231

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+16 −7
Open

Fix rir inlining promises #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 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
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
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
20 changes: 15 additions & 5 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 @@ -1097,7 +1100,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 @@ -1748,12 +1751,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 +1776,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