Emit LoadConstant as needed

Summary: We currently emit all `LoadConstant`s at the top of a function. In large functions, particularly large object literals, this can create a huge amount of register pressure in debug mode, since the literals are not moved closer to the point where they are used. To avoid this, always emit `LoadConstant` at the point where it is needed. When optimisations are enabled, we can then rely on CodeMotion to hoist and CSE to deduplicate them. Reviewed By: tmikov Differential Revision: D43048358 fbshipit-source-id: 7add8d6393f3ade22342e9e37d142d578cce3d1a
facebook · Aug 24, 2023 · 18aa617 · 18aa617
1 parent 0038645
commit 18aa617
Show file tree

Hide file tree

Showing 29 changed files with 801 additions and 742 deletions.
diff --git a/include/hermes/BCGen/HBC/Passes.h b/include/hermes/BCGen/HBC/Passes.h
@@ -29,15 +29,10 @@ class LoadConstants : public FunctionPass {
  bool operandMustBeLiteral(Instruction *Inst, unsigned opIndex);
 
  public:
- explicit LoadConstants(bool optimizationEnabled)
- : FunctionPass("LoadConstants"),
- optimizationEnabled_(optimizationEnabled) {}
+ explicit LoadConstants() : FunctionPass("LoadConstants") {}
  ~LoadConstants() override = default;
 
  bool runOnFunction(Function *F) override;
-
- private:
- bool const optimizationEnabled_;
 };
 
 class LoadParameters : public FunctionPass {

diff --git a/lib/BCGen/HBC/HBC.cpp b/lib/BCGen/HBC/HBC.cpp
@@ -74,7 +74,7 @@ void lowerIR(Module *M, const BytecodeGenerationOptions &options) {
  PM.addPass<DedupReifyArguments>();
  PM.addPass<LowerSwitchIntoJumpTables>();
  PM.addPass<SwitchLowering>();
- PM.addPass<LoadConstants>(options.optimizationEnabled);
+ PM.addPass<LoadConstants>();
  PM.addPass<LoadParameters>();
  if (options.optimizationEnabled) {
  // Lowers AllocObjects and its sequential literal properties into a single

diff --git a/lib/BCGen/HBC/Passes.cpp b/lib/BCGen/HBC/Passes.cpp
@@ -227,46 +227,43 @@ bool LoadConstants::runOnFunction(Function *F) {
  IRBuilder builder(F);
  bool changed = false;
 
- llvh::SmallDenseMap<Literal *, Instruction *, 8> constMap{};
-
- // This is a bit counter-intuitive because the logic appears reversed.
- // We only want to unique the generated literals if optimization is disabled.
- // That is the case when it causes too many registers to be generated (one
- // per literal).
- // If optimization is enabled, that is not necessary because of CSE and
- // because doing this now interefers with code motion.
- const bool uniqueLiterals = !optimizationEnabled_;
-
- auto createLoadLiteral = [&builder](Literal *literal) -> Instruction * {
+ /// Inserts and returns a load instruction for \p literal before \p where.
+ auto createLoadLiteral = [&builder](Literal *literal, Instruction *where) {
+ builder.setInsertionPoint(where);
  return llvh::isa<GlobalObject>(literal)
  ? cast<Instruction>(builder.createHBCGetGlobalObjectInst())
  : cast<Instruction>(builder.createHBCLoadConstInst(literal));
  };
 
- updateToEntryInsertionPoint(builder, F);
-
- for (BasicBlock &bbit : F->getBasicBlockList()) {
- for (auto &it : bbit.getInstList()) {
- for (unsigned i = 0, n = it.getNumOperands(); i < n; i++) {
- if (operandMustBeLiteral(&it, i))
- continue;
-
- auto *operand = llvh::dyn_cast<Literal>(it.getOperand(i));
- if (!operand)
- continue;
-
- Instruction *load = nullptr;
- if (uniqueLiterals) {
- auto &entry = constMap[operand];
- if (!entry)
- entry = createLoadLiteral(operand);
- load = entry;
- } else {
- load = createLoadLiteral(operand);
+ for (BasicBlock &BB : *F) {
+ for (auto &I : BB) {
+ if (auto *phi = llvh::dyn_cast<PhiInst>(&I)) {
+ // Since PhiInsts must always be at the start of a basic block, we have
+ // to insert the load instruction in the predecessor. This lowering is
+ // sub-optimal: for conditional branches, the load constant operation
+ // will be performed before the branch decides which path to take.
+ for (unsigned i = 0, e = phi->getNumEntries(); i < e; ++i) {
+ auto [val, bb] = phi->getEntry(i);
+ if (auto *literal = llvh::dyn_cast<Literal>(val)) {
+ auto *load = createLoadLiteral(literal, bb->getTerminator());
+ phi->updateEntry(i, load, bb);
+ changed = true;
+ }
+ }
+ continue;
+ }
+ // For all other instructions, insert load constants right before the they
+ // are needed. This minimizes their live range and therefore reduces
+ // register pressure. CodeMotion and CSE can later hoist and deduplicate
+ // them.
+ for (unsigned i = 0, e = I.getNumOperands(); i < e; ++i) {
+ if (auto *literal = llvh::dyn_cast<Literal>(I.getOperand(i))) {
+ if (!operandMustBeLiteral(&I, i)) {
+ auto *load = createLoadLiteral(literal, &I);
+ I.setOperand(load, i);
+ changed = true;
+ }
  }
-
- it.setOperand(load, i);
- changed = true;
  }
  }
  }

diff --git a/test/BCGen/HBC/async-function.js b/test/BCGen/HBC/async-function.js
@@ -57,77 +57,81 @@ var simpleAsyncFE = async function () {
 // CHECK-NEXT: Function ID 6 -> s0
 // CHECK-NEXT: Function ID 9 -> s0
 
-// CHECK:Function<global>(1 params, 9 registers, 0 symbols):
+// CHECK:Function<global>(1 params, 11 registers, 0 symbols):
 // CHECK-NEXT:Offset in debug table: source 0x0000, scope 0x0000, textified callees 0x0000
 // CHECK-NEXT: DeclareGlobalVar "simpleAsyncFE"
 // CHECK-NEXT: DeclareGlobalVar "simpleReturn"
 // CHECK-NEXT: DeclareGlobalVar "simpleAwait"
 // CHECK-NEXT: CreateEnvironment r0
-// CHECK-NEXT: GetGlobalObject r1
-// CHECK-NEXT: LoadConstUndefined r2
-// CHECK-NEXT: CreateAsyncClosure r3, r0, NCFunction<simpleReturn>
-// CHECK-NEXT: PutById r1, r3, 1, "simpleReturn"
-// CHECK-NEXT: CreateAsyncClosure r4, r0, NCFunction<simpleAwait>
-// CHECK-NEXT: PutById r1, r4, 2, "simpleAwait"
-// CHECK-NEXT: Mov r5, r2
-// CHECK-NEXT: CreateAsyncClosure r6, r0, NCFunction<simpleAsyncFE>
-// CHECK-NEXT: PutById r1, r6, 3, "simpleAsyncFE"
-// CHECK-NEXT: Mov r7, r5
-// CHECK-NEXT: Ret r7
-
-// CHECK:NCFunction<simpleReturn>(1 params, 19 registers, 0 symbols):
+// CHECK-NEXT: CreateAsyncClosure r1, r0, NCFunction<simpleReturn>
+// CHECK-NEXT: GetGlobalObject r2
+// CHECK-NEXT: PutById r2, r1, 1, "simpleReturn"
+// CHECK-NEXT: CreateAsyncClosure r3, r0, NCFunction<simpleAwait>
+// CHECK-NEXT: GetGlobalObject r4
+// CHECK-NEXT: PutById r4, r3, 2, "simpleAwait"
+// CHECK-NEXT: LoadConstUndefined r6
+// CHECK-NEXT: Mov r5, r6
+// CHECK-NEXT: CreateAsyncClosure r7, r0, NCFunction<simpleAsyncFE>
+// CHECK-NEXT: GetGlobalObject r8
+// CHECK-NEXT: PutById r8, r7, 3, "simpleAsyncFE"
+// CHECK-NEXT: Mov r9, r5
+// CHECK-NEXT: Ret r9
+
+// CHECK:NCFunction<simpleReturn>(1 params, 20 registers, 0 symbols):
 // CHECK-NEXT:Offset in debug table: source 0x001f, scope 0x0000, textified callees 0x0000
 // CHECK-NEXT: CreateEnvironment r0
 // CHECK-NEXT: LoadThisNS r1
-// CHECK-NEXT: LoadConstUndefined r2
-// CHECK-NEXT: Mov r3, r2
+// CHECK-NEXT: LoadConstUndefined r3
+// CHECK-NEXT: Mov r2, r3
 // CHECK-NEXT: CreateGeneratorClosure r4, r0, NCFunction<?anon_0_simpleReturn>
 // CHECK-NEXT: GetBuiltinClosure r5, "HermesBuiltin.spawnAsync"
-// CHECK-NEXT: ReifyArguments r3
-// CHECK-NEXT: Mov r6, r3
-// CHECK-NEXT: Mov r12, r2
-// CHECK-NEXT: Mov r11, r4
-// CHECK-NEXT: Mov r10, r1
-// CHECK-NEXT: Mov r9, r6
-// CHECK-NEXT: Call r7, r5, 4
-// CHECK-NEXT: Ret r7
+// CHECK-NEXT: ReifyArguments r2
+// CHECK-NEXT: Mov r6, r2
+// CHECK-NEXT: LoadConstUndefined r7
+// CHECK-NEXT: Mov r13, r7
+// CHECK-NEXT: Mov r12, r4
+// CHECK-NEXT: Mov r11, r1
+// CHECK-NEXT: Mov r10, r6
+// CHECK-NEXT: Call r8, r5, 4
+// CHECK-NEXT: Ret r8
 
 // CHECK:NCFunction<?anon_0_simpleReturn>(1 params, 3 registers, 0 symbols):
 // CHECK-NEXT: CreateEnvironment r0
 // CHECK-NEXT: CreateGenerator r1, r0, Function<?anon_0_?anon_0_simpleReturn>
 // CHECK-NEXT: Ret r1
 
-// CHECK:Function<?anon_0_?anon_0_simpleReturn>(1 params, 6 registers, 0 symbols):
+// CHECK:Function<?anon_0_?anon_0_simpleReturn>(1 params, 4 registers, 0 symbols):
 // CHECK-NEXT: StartGenerator
 // CHECK-NEXT: CreateEnvironment r0
+// CHECK-NEXT: ResumeGenerator r1, r0
+// CHECK-NEXT: Mov r2, r0
+// CHECK-NEXT: JmpTrue L1, r2
 // CHECK-NEXT: LoadConstUInt8 r0, 1
-// CHECK-NEXT: LoadConstUndefined r1
-// CHECK-NEXT: ResumeGenerator r3, r2
-// CHECK-NEXT: Mov r4, r2
-// CHECK-NEXT: JmpTrue L1, r4
 // CHECK-NEXT: CompleteGenerator
 // CHECK-NEXT: Ret r0
 // CHECK-NEXT:L1:
 // CHECK-NEXT: CompleteGenerator
-// CHECK-NEXT: Ret r3
+// CHECK-NEXT: Ret r1
 
-// CHECK:NCFunction<simpleAwait>(1 params, 19 registers, 1 symbols):
+// CHECK:NCFunction<simpleAwait>(1 params, 21 registers, 1 symbols):
 // CHECK-NEXT:Offset in debug table: source 0x002c, scope 0x0000, textified callees 0x0000
 // CHECK-NEXT: CreateEnvironment r0
 // CHECK-NEXT: LoadThisNS r1
-// CHECK-NEXT: LoadConstUndefined r2
-// CHECK-NEXT: Mov r3, r2
-// CHECK-NEXT: StoreNPToEnvironment r0, 0, r2
-// CHECK-NEXT: CreateGeneratorClosure r4, r0, NCFunction<?anon_0_simpleAwait>
-// CHECK-NEXT: GetBuiltinClosure r5, "HermesBuiltin.spawnAsync"
-// CHECK-NEXT: ReifyArguments r3
-// CHECK-NEXT: Mov r6, r3
-// CHECK-NEXT: Mov r12, r2
-// CHECK-NEXT: Mov r11, r4
-// CHECK-NEXT: Mov r10, r1
-// CHECK-NEXT: Mov r9, r6
-// CHECK-NEXT: Call r7, r5, 4
-// CHECK-NEXT: Ret r7
+// CHECK-NEXT: LoadConstUndefined r3
+// CHECK-NEXT: Mov r2, r3
+// CHECK-NEXT: LoadConstUndefined r4
+// CHECK-NEXT: StoreNPToEnvironment r0, 0, r4
+// CHECK-NEXT: CreateGeneratorClosure r5, r0, NCFunction<?anon_0_simpleAwait>
+// CHECK-NEXT: GetBuiltinClosure r6, "HermesBuiltin.spawnAsync"
+// CHECK-NEXT: ReifyArguments r2
+// CHECK-NEXT: Mov r7, r2
+// CHECK-NEXT: LoadConstUndefined r8
+// CHECK-NEXT: Mov r14, r8
+// CHECK-NEXT: Mov r13, r5
+// CHECK-NEXT: Mov r12, r1
+// CHECK-NEXT: Mov r11, r7
+// CHECK-NEXT: Call r9, r6, 4
+// CHECK-NEXT: Ret r9
 
 // CHECK:NCFunction<?anon_0_simpleAwait>(1 params, 4 registers, 1 symbols):
 // CHECK-NEXT: CreateEnvironment r0
@@ -136,50 +140,52 @@ var simpleAsyncFE = async function () {
 // CHECK-NEXT: CreateGenerator r2, r0, Function<?anon_0_?anon_0_simpleAwait>
 // CHECK-NEXT: Ret r2
 
-// CHECK:Function<?anon_0_?anon_0_simpleAwait>(1 params, 8 registers, 1 symbols):
+// CHECK:Function<?anon_0_?anon_0_simpleAwait>(1 params, 6 registers, 1 symbols):
 // CHECK-NEXT:Offset in debug table: source 0x0039, scope 0x0000, textified callees 0x0000
 // CHECK-NEXT: StartGenerator
 // CHECK-NEXT: CreateEnvironment r0
+// CHECK-NEXT: ResumeGenerator r2, r1
+// CHECK-NEXT: Mov r3, r1
+// CHECK-NEXT: JmpTrue L1, r3
 // CHECK-NEXT: LoadConstUndefined r1
-// CHECK-NEXT: LoadConstUInt8 r2, 2
-// CHECK-NEXT: ResumeGenerator r4, r3
-// CHECK-NEXT: Mov r5, r3
-// CHECK-NEXT: JmpTrue L1, r5
 // CHECK-NEXT: StoreNPToEnvironment r0, 0, r1
+// CHECK-NEXT: LoadConstUInt8 r4, 2
 // CHECK-NEXT: SaveGenerator L2
-// CHECK-NEXT: Ret r2
+// CHECK-NEXT: Ret r4
 // CHECK-NEXT:L2:
-// CHECK-NEXT: ResumeGenerator r5, r3
-// CHECK-NEXT: Mov r6, r3
-// CHECK-NEXT: JmpTrue L3, r6
-// CHECK-NEXT: StoreToEnvironment r0, 0, r5
-// CHECK-NEXT: LoadFromEnvironment r6, r0, 0
+// CHECK-NEXT: ResumeGenerator r1, r3
+// CHECK-NEXT: Mov r4, r3
+// CHECK-NEXT: JmpTrue L3, r4
+// CHECK-NEXT: StoreToEnvironment r0, 0, r1
+// CHECK-NEXT: LoadFromEnvironment r4, r0, 0
 // CHECK-NEXT: CompleteGenerator
-// CHECK-NEXT: Ret r6
+// CHECK-NEXT: Ret r4
 // CHECK-NEXT:L3:
 // CHECK-NEXT: CompleteGenerator
-// CHECK-NEXT: Ret r5
+// CHECK-NEXT: Ret r1
 // CHECK-NEXT:L1:
 // CHECK-NEXT: CompleteGenerator
-// CHECK-NEXT: Ret r4
+// CHECK-NEXT: Ret r2
 
-// CHECK:NCFunction<simpleAsyncFE>(1 params, 19 registers, 1 symbols):
+// CHECK:NCFunction<simpleAsyncFE>(1 params, 21 registers, 1 symbols):
 // CHECK-NEXT:Offset in debug table: source 0x004f, scope 0x0000, textified callees 0x0000
 // CHECK-NEXT: CreateEnvironment r0
 // CHECK-NEXT: LoadThisNS r1
-// CHECK-NEXT: LoadConstUndefined r2
-// CHECK-NEXT: Mov r3, r2
-// CHECK-NEXT: StoreNPToEnvironment r0, 0, r2
-// CHECK-NEXT: CreateGeneratorClosure r4, r0, NCFunction<?anon_0_simpleAsyncFE>
-// CHECK-NEXT: GetBuiltinClosure r5, "HermesBuiltin.spawnAsync"
-// CHECK-NEXT: ReifyArguments r3
-// CHECK-NEXT: Mov r6, r3
-// CHECK-NEXT: Mov r12, r2
-// CHECK-NEXT: Mov r11, r4
-// CHECK-NEXT: Mov r10, r1
-// CHECK-NEXT: Mov r9, r6
-// CHECK-NEXT: Call r7, r5, 4
-// CHECK-NEXT: Ret r7
+// CHECK-NEXT: LoadConstUndefined r3
+// CHECK-NEXT: Mov r2, r3
+// CHECK-NEXT: LoadConstUndefined r4
+// CHECK-NEXT: StoreNPToEnvironment r0, 0, r4
+// CHECK-NEXT: CreateGeneratorClosure r5, r0, NCFunction<?anon_0_simpleAsyncFE>
+// CHECK-NEXT: GetBuiltinClosure r6, "HermesBuiltin.spawnAsync"
+// CHECK-NEXT: ReifyArguments r2
+// CHECK-NEXT: Mov r7, r2
+// CHECK-NEXT: LoadConstUndefined r8
+// CHECK-NEXT: Mov r14, r8
+// CHECK-NEXT: Mov r13, r5
+// CHECK-NEXT: Mov r12, r1
+// CHECK-NEXT: Mov r11, r7
+// CHECK-NEXT: Call r9, r6, 4
+// CHECK-NEXT: Ret r9
 
 // CHECK:NCFunction<?anon_0_simpleAsyncFE>(1 params, 4 registers, 1 symbols):
 // CHECK-NEXT: CreateEnvironment r0
@@ -188,32 +194,32 @@ var simpleAsyncFE = async function () {
 // CHECK-NEXT: CreateGenerator r2, r0, Function<?anon_0_?anon_0_simpleAsyncFE>
 // CHECK-NEXT: Ret r2
 
-// CHECK:Function<?anon_0_?anon_0_simpleAsyncFE>(1 params, 8 registers, 1 symbols):
+// CHECK:Function<?anon_0_?anon_0_simpleAsyncFE>(1 params, 6 registers, 1 symbols):
 // CHECK-NEXT:Offset in debug table: source 0x005c, scope 0x0000, textified callees 0x0000
 // CHECK-NEXT: StartGenerator
 // CHECK-NEXT: CreateEnvironment r0
+// CHECK-NEXT: ResumeGenerator r2, r1
+// CHECK-NEXT: Mov r3, r1
+// CHECK-NEXT: JmpTrue L1, r3
 // CHECK-NEXT: LoadConstUndefined r1
-// CHECK-NEXT: LoadConstUInt8 r2, 2
-// CHECK-NEXT: ResumeGenerator r4, r3
-// CHECK-NEXT: Mov r5, r3
-// CHECK-NEXT: JmpTrue L1, r5
 // CHECK-NEXT: StoreNPToEnvironment r0, 0, r1
+// CHECK-NEXT: LoadConstUInt8 r4, 2
 // CHECK-NEXT: SaveGenerator L2
-// CHECK-NEXT: Ret r2
+// CHECK-NEXT: Ret r4
 // CHECK-NEXT:L2:
-// CHECK-NEXT: ResumeGenerator r5, r3
-// CHECK-NEXT: Mov r6, r3
-// CHECK-NEXT: JmpTrue L3, r6
-// CHECK-NEXT: StoreToEnvironment r0, 0, r5
-// CHECK-NEXT: LoadFromEnvironment r6, r0, 0
+// CHECK-NEXT: ResumeGenerator r1, r3
+// CHECK-NEXT: Mov r4, r3
+// CHECK-NEXT: JmpTrue L3, r4
+// CHECK-NEXT: StoreToEnvironment r0, 0, r1
+// CHECK-NEXT: LoadFromEnvironment r4, r0, 0
 // CHECK-NEXT: CompleteGenerator
-// CHECK-NEXT: Ret r6
+// CHECK-NEXT: Ret r4
 // CHECK-NEXT:L3:
 // CHECK-NEXT: CompleteGenerator
-// CHECK-NEXT: Ret r5
+// CHECK-NEXT: Ret r1
 // CHECK-NEXT:L1:
 // CHECK-NEXT: CompleteGenerator
-// CHECK-NEXT: Ret r4
+// CHECK-NEXT: Ret r2
 
 // CHECK:Debug filename table:
 // CHECK-NEXT: 0: {{.*}}async-function.js
@@ -223,18 +229,18 @@ var simpleAsyncFE = async function () {
 
 // CHECK:Debug source table:
 // CHECK-NEXT: 0x0000 function idx 0, starts at line 10 col 1
-// CHECK-NEXT: bc 26: line 10 col 1 scope offset 0x0000 env none
+// CHECK-NEXT: bc 24: line 10 col 1 scope offset 0x0000 env none
 // CHECK-NEXT: bc 37: line 10 col 1 scope offset 0x0000 env none
-// CHECK-NEXT: bc 51: line 19 col 19 scope offset 0x0000 env none
+// CHECK-NEXT: bc 55: line 19 col 19 scope offset 0x0000 env none
 // CHECK-NEXT: 0x001f function idx 1, starts at line 10 col 1
-// CHECK-NEXT: bc 34: line 10 col 1 scope offset 0x0000 env none
+// CHECK-NEXT: bc 36: line 10 col 1 scope offset 0x0000 env none
 // CHECK-NEXT: 0x002c function idx 4, starts at line 14 col 1
-// CHECK-NEXT: bc 38: line 14 col 1 scope offset 0x0000 env none
+// CHECK-NEXT: bc 42: line 14 col 1 scope offset 0x0000 env none
 // CHECK-NEXT: 0x0039 function idx 6, starts at line 14 col 1
 // CHECK-NEXT: bc 21: line 15 col 11 scope offset 0x0000 env none
 // CHECK-NEXT: bc 25: line 15 col 11 scope offset 0x0000 env none
 // CHECK-NEXT: 0x004f function idx 7, starts at line 19 col 21
-// CHECK-NEXT: bc 38: line 19 col 21 scope offset 0x0000 env none
+// CHECK-NEXT: bc 42: line 19 col 21 scope offset 0x0000 env none
 // CHECK-NEXT: 0x005c function idx 9, starts at line 19 col 21
 // CHECK-NEXT: bc 21: line 20 col 11 scope offset 0x0000 env none
 // CHECK-NEXT: bc 25: line 20 col 11 scope offset 0x0000 env none