CSE for requires calls.

Summary:
Previous work in this stack on require calls has focused on making
them faster, by caching the results for a particular module index.
This cache is global to an execution.  This diff attempts to identify
redundant require calls statically within a function: essentially,
making "require(n)" subject to CSE, by noting that require has special
semantics that allow this.  In particular, require(n) is idempotent
(wrt subsequent calls for the same value of n): the first call may
have side effects, but the second and subsequent calls do not.  We
make a special case for [metro-require] calls in CSEValue::canHandle
to reflect this assumption.

I add a statistic that breaks out the number of "require CSEs".

I also considered doing a specialized optimization for require calls
in Instruction::isIdenticalTo: instructions only get the
[metro-require] attribute if we've proved that their first argument is
the metroRequire function.  So we'd assume the first argument to the
all such calls are semantically the same in isIdentical (and in the
hash function).

This yielded a small number of additional require CSEs: on the order
of 25 to 30.  I give a range here because, try as I might, doing this
made the results (in terms of # of CSE) non-deterministic.  I was
unable to figure out why -- I took care to make sure the hash function
gave the same result whenever isIdentical would return true.  I
eventually decided that figuring out the mystery was not worth it,
given the small extra benefit.

Design question: in canHandle, we make a special case for
metro-require.  This seems perhaps less general than it might.  In
that function, we would all a call to be handled if it was labeled
[pure].  The metro-require case is "as if pure"; for a given module
Id, it has side effects, but only on the first call globally.  So
maybe we should have an [idempotent] attribute, in case we identify
other functions with this property, and generalize canHandle in this
way (pure || idempotent)?  (The special assumption of equivalent first
arguments is, however, specific to metro-require calls.)

Reviewed By: neildhar

Differential Revision: D64773199

fbshipit-source-id: d78538ac2cc12f10e6cfbaa4cc0879a826d09365

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<TerminatorInst>(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;

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

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