Disable lowering to LoadThisNS when optimizations are disabled

Summary: We currently have extremely limited coverage of testing CoerceThisNS, because it is almost always merged with LoadParam, or eliminated entirely. Given that merging it with LoadParam is purely an optimization, disable it when we are building without optimizations. Reviewed By: fbmal7 Differential Revision: D69508165 fbshipit-source-id: 2d6b5f7fd08e4464a253ba2342a43d7187d35f5e
facebook · Feb 26, 2025 · cc5a927 · cc5a927
1 parent 352c7d0
commit cc5a927
Show file tree

Hide file tree

Showing 9 changed files with 49 additions and 23 deletions.
diff --git a/include/hermes/BCGen/HBC/Passes/PeepholeLowering.h b/include/hermes/BCGen/HBC/Passes/PeepholeLowering.h
@@ -15,12 +15,7 @@
 namespace hermes {
 namespace hbc {
 
-class PeepholeLowering : public FunctionPass {
- public:
-  explicit PeepholeLowering() : FunctionPass("PeepholeLowering") {}
-
-  bool runOnFunction(Function *F) override;
-};
+Pass *createPeepholeLowering(bool optimize);
 
 } // namespace hbc
 } // namespace hermes

diff --git a/lib/BCGen/HBC/LoweringPipelines.cpp b/lib/BCGen/HBC/LoweringPipelines.cpp
@@ -52,7 +52,7 @@ void lowerModuleIR(Module *M, const BytecodeGenerationOptions &options) {
   // Lowering ExponentiationOperator and ThrowTypeError (in PeepholeLowering)
   // needs to run before LowerBuiltinCalls because it introduces calls to
   // HermesInternal.
-  PM.addPass(new PeepholeLowering());
+  PM.addPass(createPeepholeLowering(options.optimizationEnabled));
   // LowerBuilinCalls needs to run before the rest of the lowering.
   PM.addPass(createLowerBuiltinCalls());
   // Turn Calls into CallNs.

diff --git a/lib/BCGen/HBC/Passes/PeepholeLowering.cpp b/lib/BCGen/HBC/Passes/PeepholeLowering.cpp
@@ -17,9 +17,10 @@ class DoLower {
   Function *const F_;
   IRBuilder builder_{F_};
   IRBuilder::InstructionDestroyer destroyer_{};
+  bool optimize_;
 
  public:
-  explicit DoLower(Function *F) : F_(F) {}
+  explicit DoLower(Function *F, bool optimize) : F_(F), optimize_(optimize) {}
 
   bool run() {
     bool changed = false;
@@ -45,6 +46,11 @@ class DoLower {
   Value *peep(Instruction *I) {
     switch (I->getKind()) {
       case ValueKind::CoerceThisNSInstKind:
+        // This transformation is purely an optimization to collapse a sequence
+        // of LoadParam + CoerceThisNS into a LoadThisNS, so skip it if
+        // optimizations are disabled.
+        if (!optimize_)
+          return nullptr;
         return lowerCoerceThisNSInst(
             llvh::cast<CoerceThisNSInst>(I), builder_, destroyer_);
       case ValueKind::BinaryExponentiationInstKind:
@@ -130,8 +136,18 @@ class DoLower {
   }
 };
 
-bool PeepholeLowering::runOnFunction(Function *F) {
-  return DoLower(F).run();
+Pass *createPeepholeLowering(bool optimize) {
+  class ThisPass : public FunctionPass {
+    bool optimize_;
+
+   public:
+    explicit ThisPass(bool optimize)
+        : FunctionPass("PeepholeLowering"), optimize_(optimize) {}
+    bool runOnFunction(Function *F) override {
+      return DoLower(F, optimize_).run();
+    }
+  };
+  return new ThisPass(optimize);
 }
 
 } // namespace hbc

diff --git a/lib/BCGen/SH/LoweringPasses.h b/lib/BCGen/SH/LoweringPasses.h
@@ -32,7 +32,7 @@ Pass *createLowerCalls(SHRegisterAllocator &RA);
 Pass *createRecreateCheapValues(SHRegisterAllocator &RA);
 
 /// Perform simple peephole lowering.
-Pass *createPeepholeLowering();
+Pass *createPeepholeLowering(bool optimize);
 } // namespace hermes::sh
 
 #endif
diff --git a/lib/BCGen/SH/PeepholeLowering.cpp b/lib/BCGen/SH/PeepholeLowering.cpp
@@ -17,9 +17,11 @@ class PeepholeLowering {
   Function *const F_;
   IRBuilder builder_{F_};
   IRBuilder::InstructionDestroyer destroyer_{};
+  bool optimize_;
 
  public:
-  explicit PeepholeLowering(Function *F) : F_(F) {}
+  explicit PeepholeLowering(Function *F, bool optimize)
+      : F_(F), optimize_(optimize) {}
 
   bool run() {
     bool changed = false;
@@ -45,6 +47,11 @@ class PeepholeLowering {
   Value *peep(Instruction *I) {
     switch (I->getKind()) {
       case ValueKind::CoerceThisNSInstKind: {
+        // This transformation is purely an optimization to collapse a sequence
+        // of LoadParam + CoerceThisNS into a LoadThisNS, so skip it if
+        // optimizations are disabled.
+        if (!optimize_)
+          return nullptr;
         return lowerCoerceThisNSInst(
             llvh::cast<CoerceThisNSInst>(I), builder_, destroyer_);
       }
@@ -70,15 +77,18 @@ class PeepholeLowering {
 
 } // namespace
 
-Pass *createPeepholeLowering() {
+Pass *createPeepholeLowering(bool optimize) {
   class ThisPass : public FunctionPass {
+    bool optimize_;
+
    public:
-    explicit ThisPass() : FunctionPass("PeepholeLowering") {}
+    explicit ThisPass(bool optimize)
+        : FunctionPass("PeepholeLowering"), optimize_(optimize) {}
     bool runOnFunction(Function *F) override {
-      return PeepholeLowering(F).run();
+      return PeepholeLowering(F, optimize_).run();
     }
   };
-  return new ThisPass();
+  return new ThisPass(optimize);
 }
 
 } // namespace hermes::sh
diff --git a/lib/BCGen/SH/SH.cpp b/lib/BCGen/SH/SH.cpp
@@ -2461,7 +2461,7 @@ bool lowerModuleIR(Module *M, bool optimize) {
   // Lowering ExponentiationOperator and ThrowTypeError (in PeepholeLowering)
   // needs to run before LowerBuiltinCalls because it introduces calls to
   // HermesInternal.
-  PM.addPass(sh::createPeepholeLowering());
+  PM.addPass(sh::createPeepholeLowering(optimize));
   // LowerBuiltinCalls needs to run before the rest of the lowering.
   PM.addPass(createLowerBuiltinCalls());
   PM.addPass(new LowerNumericProperties());

diff --git a/test/BCGen/HBC/async-function.js b/test/BCGen/HBC/async-function.js
@@ -95,7 +95,8 @@ var simpleAsyncFE = async function () {
 // CHECK-NEXT:Offset in debug table: source 0x0016, lexical 0x0000
 // CHECK-NEXT:    LoadConstUndefined r4
 // CHECK-NEXT:    Mov               r5, r4
-// CHECK-NEXT:    LoadThisNS        r4
+// CHECK-NEXT:    LoadParam         r4, 0
+// CHECK-NEXT:    CoerceThisNS      r4, r4
 // CHECK-NEXT:    GetParentEnvironment r3, 0
 // CHECK-NEXT:    CreateEnvironment r3, r3, 0
 // CHECK-NEXT:    CreateClosure     r3, r3, NCFunction<?anon_0_simpleReturn>
@@ -109,7 +110,8 @@ var simpleAsyncFE = async function () {
 // CHECK-NEXT:Offset in debug table: source 0x001d, lexical 0x0000
 // CHECK-NEXT:    LoadConstUndefined r4
 // CHECK-NEXT:    Mov               r5, r4
-// CHECK-NEXT:    LoadThisNS        r4
+// CHECK-NEXT:    LoadParam         r4, 0
+// CHECK-NEXT:    CoerceThisNS      r4, r4
 // CHECK-NEXT:    GetParentEnvironment r3, 0
 // CHECK-NEXT:    CreateEnvironment r3, r3, 0
 // CHECK-NEXT:    CreateClosure     r3, r3, NCFunction<?anon_0_simpleAwait>
@@ -123,7 +125,8 @@ var simpleAsyncFE = async function () {
 // CHECK-NEXT:Offset in debug table: source 0x0024, lexical 0x0000
 // CHECK-NEXT:    LoadConstUndefined r4
 // CHECK-NEXT:    Mov               r5, r4
-// CHECK-NEXT:    LoadThisNS        r4
+// CHECK-NEXT:    LoadParam         r4, 0
+// CHECK-NEXT:    CoerceThisNS      r4, r4
 // CHECK-NEXT:    GetParentEnvironment r3, 0
 // CHECK-NEXT:    CreateEnvironment r3, r3, 0
 // CHECK-NEXT:    CreateClosure     r3, r3, NCFunction<?anon_0_simpleAsyncFE>
@@ -481,11 +484,11 @@ var simpleAsyncFE = async function () {
 // CHECK-NEXT:    bc 41: line 10 col 1
 // CHECK-NEXT:    bc 59: line 19 col 19
 // CHECK-NEXT:  0x0016  function idx 1, starts at line 10 col 1
-// CHECK-NEXT:    bc 29: line 10 col 1
+// CHECK-NEXT:    bc 33: line 10 col 1
 // CHECK-NEXT:  0x001d  function idx 2, starts at line 14 col 1
-// CHECK-NEXT:    bc 29: line 14 col 1
+// CHECK-NEXT:    bc 33: line 14 col 1
 // CHECK-NEXT:  0x0024  function idx 3, starts at line 19 col 21
-// CHECK-NEXT:    bc 29: line 19 col 21
+// CHECK-NEXT:    bc 33: line 19 col 21
 // CHECK-NEXT:  0x002b  function idx 8, starts at line 14 col 1
 // CHECK-NEXT:    bc 169: line 15 col 11
 // CHECK-NEXT:    bc 337: line 15 col 11

diff --git a/test/hermes/load-this.js b/test/hermes/load-this.js
@@ -6,6 +6,7 @@
  */
 
 // RUN: %hermes -non-strict -target=HBC -O %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -non-strict -target=HBC -O0 -lazy %s | %FileCheck --match-full-lines %s
 // RUN: %shermes -exec %s | %FileCheck --match-full-lines %s
 
 function strictFunc() {

diff --git a/test/hermes/reflect.js b/test/hermes/reflect.js
@@ -6,6 +6,7 @@
  */
 
 // RUN: %hermes -Xhermes-internal-test-methods -Xes6-proxy -non-strict -O -target=HBC %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -Xhermes-internal-test-methods -Xes6-proxy -non-strict -O0 -target=HBC -lazy %s | %FileCheck --match-full-lines %s
 
 function betterToString(value) {
   if (Array.isArray(value)) {