From 289d302593055e8c31966dd122efcaf4a7d0d288 Mon Sep 17 00:00:00 2001 From: Neil Dhar Date: Tue, 4 Feb 2025 17:09:12 -0800 Subject: [PATCH] Track IR function of operand to GetClosureScopeInst Summary: We have to track the IR function of the closure operand so that we can find the `GetClosureScopeInst` by iterating users of the `Function`. This is useful if we want to update the parent of that function for instance. Reviewed By: avp Differential Revision: D66236886 fbshipit-source-id: 393cdd9ff9e12482f380800ac0c6f04891bbf85d --- doc/IR.md | 4 ++-- include/hermes/IR/IRBuilder.h | 5 ++--- include/hermes/IR/Instrs.h | 13 ++++++++++--- lib/IR/IRBuilder.cpp | 3 ++- lib/IR/IRVerifier.cpp | 9 +++++++++ lib/Optimizer/Scalar/FunctionAnalysis.cpp | 9 +++++++++ lib/Optimizer/Scalar/Inlining.cpp | 2 +- test/Optimizer/inline-unavailable-env.js | 2 +- 8 files changed, 36 insertions(+), 11 deletions(-) diff --git a/doc/IR.md b/doc/IR.md index 2a246d3c58c..2c76cd5bdb7 100644 --- a/doc/IR.md +++ b/doc/IR.md @@ -374,8 +374,8 @@ Effects | Does not read or write to memory. GetClosureScopeInst | _ --- | --- | Description | Retrieve the scope from the given closure. -Example | %0 = GetClosureScopeInst %varScope, %closure -Arguments | %varScope is the VariableScope that describes the resulting scope. %closure is the closure from which to read the scope. +Example | %0 = GetClosureScopeInst %varScope, %function, %closure +Arguments | %varScope is the VariableScope that describes the resulting scope. %function is the IR function that the closure operand is known to refer to. %closure is the closure from which to read the scope. Semantics | The instruction returns the scope stored in the given closure. Effects | Does not read or write to memory. diff --git a/include/hermes/IR/IRBuilder.h b/include/hermes/IR/IRBuilder.h index 5f2cb76cfa8..ee43aa6230f 100644 --- a/include/hermes/IR/IRBuilder.h +++ b/include/hermes/IR/IRBuilder.h @@ -327,9 +327,8 @@ class IRBuilder { Instruction *startScope, LiteralNumber *numLevels); - GetClosureScopeInst *createGetClosureScopeInst( - VariableScope *scope, - Value *closure); + GetClosureScopeInst * + createGetClosureScopeInst(VariableScope *scope, Function *F, Value *closure); LoadStackInst *createLoadStackInst(AllocStackInst *ptr); diff --git a/include/hermes/IR/Instrs.h b/include/hermes/IR/Instrs.h index 94ec5b2072c..b873ae4de51 100644 --- a/include/hermes/IR/Instrs.h +++ b/include/hermes/IR/Instrs.h @@ -786,11 +786,15 @@ class GetClosureScopeInst : public BaseScopeInst { GetClosureScopeInst(const GetClosureScopeInst &) = delete; void operator=(const GetClosureScopeInst &) = delete; - enum { ClosureIdx = BaseScopeInst::LAST_IDX }; + enum { FunctionCodeIdx = BaseScopeInst::LAST_IDX, ClosureIdx }; public: - explicit GetClosureScopeInst(VariableScope *varScope, Value *closure) + explicit GetClosureScopeInst( + VariableScope *varScope, + Function *F, + Value *closure) : BaseScopeInst(ValueKind::GetClosureScopeInstKind, varScope) { + pushOperand(F); pushOperand(closure); } explicit GetClosureScopeInst( @@ -798,9 +802,12 @@ class GetClosureScopeInst : public BaseScopeInst { llvh::ArrayRef operands) : BaseScopeInst(src, operands) {} - Value *getClosure() { + Value *getClosure() const { return cast(getOperand(ClosureIdx)); } + Function *getFunctionCode() const { + return cast(getOperand(FunctionCodeIdx)); + } SideEffect getSideEffectImpl() const { return SideEffect{}.setIdempotent(); diff --git a/lib/IR/IRBuilder.cpp b/lib/IR/IRBuilder.cpp index 69f43217e3c..3a20cd7529f 100644 --- a/lib/IR/IRBuilder.cpp +++ b/lib/IR/IRBuilder.cpp @@ -409,8 +409,9 @@ LIRResolveScopeInst *IRBuilder::createLIRResolveScopeInst( GetClosureScopeInst *IRBuilder::createGetClosureScopeInst( VariableScope *scope, + Function *F, Value *closure) { - auto *GCSI = new GetClosureScopeInst(scope, closure); + auto *GCSI = new GetClosureScopeInst(scope, F, closure); insert(GCSI); return GCSI; } diff --git a/lib/IR/IRVerifier.cpp b/lib/IR/IRVerifier.cpp index 8d911c25693..5c87985abd4 100644 --- a/lib/IR/IRVerifier.cpp +++ b/lib/IR/IRVerifier.cpp @@ -1389,6 +1389,15 @@ bool Verifier::visitLIRResolveScopeInst( } bool Verifier::visitGetClosureScopeInst( const hermes::GetClosureScopeInst &Inst) { + for (auto *U : Inst.getFunctionCode()->getUsers()) { + if (auto *BCLI = llvh::dyn_cast(U)) { + AssertIWithMsg( + Inst, + BCLI->getVarScope() == Inst.getVariableScope(), + "Scope result does not match function creation"); + return true; + } + } return true; } diff --git a/lib/Optimizer/Scalar/FunctionAnalysis.cpp b/lib/Optimizer/Scalar/FunctionAnalysis.cpp index 4f52f31ed96..ba1c2d0de71 100644 --- a/lib/Optimizer/Scalar/FunctionAnalysis.cpp +++ b/lib/Optimizer/Scalar/FunctionAnalysis.cpp @@ -378,6 +378,15 @@ void analyzeFunctionCallsites(Function *F) { continue; } + if (auto *GCSI = llvh::dyn_cast(user)) { + // Ignore uses in GetClosureScopeInst. + (void)GCSI; + assert( + GCSI->getFunctionCode() == F && + "invalid use of Function as operand of GetClosureScopeInst"); + continue; + } + // Unknown user of Function. LLVM_DEBUG( llvh::dbgs() << "Unknown function user: " << user->getKindStr() diff --git a/lib/Optimizer/Scalar/Inlining.cpp b/lib/Optimizer/Scalar/Inlining.cpp index 470607a0ccf..05ca5c7c5f8 100644 --- a/lib/Optimizer/Scalar/Inlining.cpp +++ b/lib/Optimizer/Scalar/Inlining.cpp @@ -352,7 +352,7 @@ static Value *inlineFunction( operandMap[&I] = callScope; } else { operandMap[&I] = builder.createGetClosureScopeInst( - GPS->getVariableScope(), CI->getCallee()); + GPS->getVariableScope(), F, CI->getCallee()); } continue; } diff --git a/test/Optimizer/inline-unavailable-env.js b/test/Optimizer/inline-unavailable-env.js index 3c99435556d..687717fa4bd 100644 --- a/test/Optimizer/inline-unavailable-env.js +++ b/test/Optimizer/inline-unavailable-env.js @@ -56,7 +56,7 @@ function foo(sink){ // CHECK-NEXT:%BB1: // CHECK-NEXT: ThrowTypeErrorInst "Trying to call a non-function": string // CHECK-NEXT:%BB2: -// CHECK-NEXT: %11 = GetClosureScopeInst (:environment) %VS2: any, %7: undefined|object +// CHECK-NEXT: %11 = GetClosureScopeInst (:environment) %VS2: any, %x(): functionCode, %7: undefined|object // CHECK-NEXT: %12 = LoadFrameInst (:number) %11: environment, [%VS2.y]: number // CHECK-NEXT: %13 = FAddInst (:number) %12: number, 1: number // CHECK-NEXT: StoreFrameInst %11: environment, %13: number, [%VS2.y]: number