diff --git a/lib/Optimizer/Scalar/CSE.cpp b/lib/Optimizer/Scalar/CSE.cpp index 41ede4315c6..ad970b3a737 100644 --- a/lib/Optimizer/Scalar/CSE.cpp +++ b/lib/Optimizer/Scalar/CSE.cpp @@ -23,6 +23,7 @@ using namespace hermes; STATISTIC(NumCSE, "Number of instructions CSE'd"); +STATISTIC(NumRequireCSE, "Number of require calls CSE'd"); //===----------------------------------------------------------------------===// // Simple Value @@ -45,6 +46,19 @@ struct CSEValue { /// Return true if we know how to CSE this instruction. static bool canHandle(Instruction *Inst) { + // Calls to metro's require function are idempotent (for a given module id). + // Consider the heap separated into a portion private to require (i.e., + // the data structure that keeps track of what modules have been + // initialized), and the rest of the heap. The module init functions + // can access and update the "rest of the heap", but not the data stucture + // private to require (more specifically, the portion of that data stucture + // relevant to the given module id). Require uses its private data + // structure to ensure that the module init function runs only on the first + // call for a module id. Thus, require calls as a whole are idempotent. + if (Inst->getAttributesRef(Inst->getModule()).isMetroRequire) { + assert(Inst->getKind() == ValueKind::CallInstKind); + return true; + } // Check that the instruction can be freely reordered and deduplicated, and // that it is not a terminator. return !llvh::isa(Inst) && Inst->getSideEffect().isPure() && @@ -160,6 +174,11 @@ bool CSEContext::processNode(StackNode *SN) { // Now that we know we have an instruction we understand see if the // instruction has an available value. If so, use it. if (Value *V = availableValues_.lookup(&Inst)) { + if (AreStatisticsEnabled() && + Inst.getAttributes(Inst.getModule()).isMetroRequire) { + NumRequireCSE++; + } + Inst.replaceAllUsesWith(V); destroyer.add(&Inst); changed = true; diff --git a/test/Optimizer/xmod-require-cse.js b/test/Optimizer/xmod-require-cse.js new file mode 100644 index 00000000000..14a294d1727 --- /dev/null +++ b/test/Optimizer/xmod-require-cse.js @@ -0,0 +1,75 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// RUN: %hermesc -O -dump-ir %s | %FileCheckOrRegen --match-full-lines %s + +// This test shows that require calls with the same module index are CSE'd +// a dominating occurrence will be used for a subsequent occurrence. + +function fakeRequire(n) { + switch (n) { + case 7: + return {blah: 100, bz: 200}; + case 8: + return {foo: 1000}; + default: + return null; + } +} + +print($SHBuiltin.moduleFactory( + 0, + function(glob, require) { + // The second require(7) should not have a call -- it should + // re-use the result of the first. + return require(7).blah + require(8).foo + require(7).baz; + })(undefined, fakeRequire)); + +// Auto-generated content below. Please do not modify manually. + +// CHECK:scope %VS0 [] + +// CHECK:function global(): any +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = CreateScopeInst (:environment) %VS0: any, empty: any +// CHECK-NEXT: DeclareGlobalVarInst "fakeRequire": string +// CHECK-NEXT: %2 = CreateFunctionInst (:object) %0: environment, %fakeRequire(): functionCode +// CHECK-NEXT: StorePropertyLooseInst %2: object, globalObject: object, "fakeRequire": string +// CHECK-NEXT: %4 = TryLoadGlobalPropertyInst (:any) globalObject: object, "print": string +// CHECK-NEXT: %5 = CreateFunctionInst (:object) %0: environment, %""(): functionCode +// CHECK-NEXT: %6 = LoadPropertyInst (:any) globalObject: object, "fakeRequire": string +// CHECK-NEXT: %7 = CallInst (:string|number|bigint) %5: object, %""(): functionCode, true: boolean, empty: any, undefined: undefined, undefined: undefined, undefined: undefined, %6: any +// CHECK-NEXT: %8 = CallInst (:any) %4: any, empty: any, false: boolean, empty: any, undefined: undefined, undefined: undefined, %7: string|number|bigint +// CHECK-NEXT: ReturnInst %8: any +// CHECK-NEXT:function_end + +// CHECK:function fakeRequire(n: any): null|object +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = LoadParamInst (:any) %n: any +// CHECK-NEXT: SwitchInst %0: any, %BB3, 7: number, %BB1, 8: number, %BB2 +// CHECK-NEXT:%BB1: +// CHECK-NEXT: %2 = AllocObjectLiteralInst (:object) empty: any, "blah": string, 100: number, "bz": string, 200: number +// CHECK-NEXT: ReturnInst %2: object +// CHECK-NEXT:%BB2: +// CHECK-NEXT: %4 = AllocObjectLiteralInst (:object) empty: any, "foo": string, 1000: number +// CHECK-NEXT: ReturnInst %4: object +// CHECK-NEXT:%BB3: +// CHECK-NEXT: ReturnInst null: null +// CHECK-NEXT:function_end + +// CHECK:function ""(glob: any, require: any): string|number|bigint [allCallsitesKnownInStrictMode] +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = LoadParamInst (:any) %require: any +// CHECK-NEXT: %1 = CallInst [metro-require] (:any) %0: any, empty: any, false: boolean, empty: any, undefined: undefined, undefined: undefined, 7: number +// CHECK-NEXT: %2 = LoadPropertyInst (:any) %1: any, "blah": string +// CHECK-NEXT: %3 = CallInst [metro-require] (:any) %0: any, empty: any, false: boolean, empty: any, undefined: undefined, undefined: undefined, 8: number +// CHECK-NEXT: %4 = LoadPropertyInst (:any) %3: any, "foo": string +// CHECK-NEXT: %5 = BinaryAddInst (:string|number|bigint) %2: any, %4: any +// CHECK-NEXT: %6 = LoadPropertyInst (:any) %1: any, "baz": string +// CHECK-NEXT: %7 = BinaryAddInst (:string|number|bigint) %5: string|number|bigint, %6: any +// CHECK-NEXT: ReturnInst %7: string|number|bigint +// CHECK-NEXT:function_end diff --git a/test/Optimizer/xmod-requires-opt.js b/test/Optimizer/xmod-requires-opt.js index 4fc775332bd..aa536332426 100644 --- a/test/Optimizer/xmod-requires-opt.js +++ b/test/Optimizer/xmod-requires-opt.js @@ -7,11 +7,11 @@ // RUN: %hermesc -O -dump-ir %s | %FileCheckOrRegen --match-full-lines %s -// This tests simulates the Metro requires tranform, and shows that +// This test simulates the Metro requires tranform, and shows that // requires calls in a module are compiled to a bytecode instruction. // The important case is the call in the module factory function -// modFact1, which does "require(0)" (twice). These calls should be +// modFact1, which calls "require(0)". This call should be // annotated with the [metro-require] attribute. function require(modIdx) { @@ -31,9 +31,8 @@ function require(modIdx) { return $SHBuiltin.moduleFactory( 1, function modFact1(global, require, module, exports) { - // The first one of these should be a cache miss, the - // second a cache hit. - var x = require(0).bar() + require(0).bar(); + // The require(0) should be annotated as a [metro-require]. + var x = require(0).bar(); exports.x = x; return exports; })(undefined, require, mod, exports); @@ -94,12 +93,8 @@ function require(modIdx) { // CHECK-NEXT: %2 = CallInst [metro-require] (:any) %0: any, empty: any, false: boolean, empty: any, undefined: undefined, undefined: undefined, 0: number // CHECK-NEXT: %3 = LoadPropertyInst (:any) %2: any, "bar": string // CHECK-NEXT: %4 = CallInst (:any) %3: any, empty: any, false: boolean, empty: any, undefined: undefined, %2: any -// CHECK-NEXT: %5 = CallInst [metro-require] (:any) %0: any, empty: any, false: boolean, empty: any, undefined: undefined, undefined: undefined, 0: number -// CHECK-NEXT: %6 = LoadPropertyInst (:any) %5: any, "bar": string -// CHECK-NEXT: %7 = CallInst (:any) %6: any, empty: any, false: boolean, empty: any, undefined: undefined, %5: any -// CHECK-NEXT: %8 = BinaryAddInst (:string|number|bigint) %4: any, %7: any -// CHECK-NEXT: StorePropertyLooseInst %8: string|number|bigint, %1: any, "x": string -// CHECK-NEXT: ReturnInst %1: any +// CHECK-NEXT: StorePropertyLooseInst %4: any, %1: any, "x": string +// CHECK-NEXT: ReturnInst %1: any // CHECK-NEXT:function_end // CHECK:function bar(): number