From 3f04d18ef7ea3b287df907eaae04eba6ec207f56 Mon Sep 17 00:00:00 2001
From: Aliaksandr Kalenik <kalenik.aliaksandr@gmail.com>
Date: Thu, 24 Apr 2025 22:10:41 +0200
Subject: [PATCH] LibJS: Add new operand type for function arguments

This allows us to directly access passed arguments instead of copying
them to register/local first using GetArgument instruction.
---
 Libraries/LibJS/Bytecode/Executable.h    |  1 +
 Libraries/LibJS/Bytecode/Generator.cpp   | 28 +++++++--------
 Libraries/LibJS/Bytecode/Instruction.h   |  2 --
 Libraries/LibJS/Bytecode/Interpreter.cpp | 32 ++++-------------
 Libraries/LibJS/Bytecode/Interpreter.h   |  6 ++--
 Libraries/LibJS/Bytecode/Op.h            | 46 ------------------------
 Libraries/LibJS/Bytecode/Operand.h       |  5 +--
 7 files changed, 27 insertions(+), 93 deletions(-)

diff --git a/Libraries/LibJS/Bytecode/Executable.h b/Libraries/LibJS/Bytecode/Executable.h
index d60268980c5..6354b374d3a 100644
--- a/Libraries/LibJS/Bytecode/Executable.h
+++ b/Libraries/LibJS/Bytecode/Executable.h
@@ -87,6 +87,7 @@ public:
 
     Vector<FlyString> local_variable_names;
     size_t local_index_base { 0 };
+    size_t argument_index_base { 0 };
 
     Optional<IdentifierTableIndex> length_identifier;
 
diff --git a/Libraries/LibJS/Bytecode/Generator.cpp b/Libraries/LibJS/Bytecode/Generator.cpp
index 0bd16a12e46..de43c27ed7d 100644
--- a/Libraries/LibJS/Bytecode/Generator.cpp
+++ b/Libraries/LibJS/Bytecode/Generator.cpp
@@ -72,15 +72,13 @@ CodeGenerationErrorOr<void> Generator::emit_function_declaration_instantiation(E
         auto const& parameter = formal_parameters.parameters()[param_index];
 
         if (parameter.is_rest) {
-            auto argument_reg = allocate_register();
-            emit<Op::CreateRestParams>(argument_reg.operand(), param_index);
-            emit<Op::SetArgument>(param_index, argument_reg.operand());
+            emit<Op::CreateRestParams>(Operand { Operand::Type::Argument, param_index }, param_index);
         } else if (parameter.default_value) {
             auto& if_undefined_block = make_block();
             auto& if_not_undefined_block = make_block();
 
             auto argument_reg = allocate_register();
-            emit<Op::GetArgument>(argument_reg.operand(), param_index);
+            emit<Op::Mov>(argument_reg.operand(), Operand { Operand::Type::Argument, param_index });
 
             emit<Op::JumpUndefined>(
                 argument_reg.operand(),
@@ -89,7 +87,7 @@ CodeGenerationErrorOr<void> Generator::emit_function_declaration_instantiation(E
 
             switch_to_basic_block(if_undefined_block);
             auto operand = TRY(parameter.default_value->generate_bytecode(*this));
-            emit<Op::SetArgument>(param_index, *operand);
+            emit<Op::Mov>(Operand { Operand::Type::Argument, param_index }, *operand);
             emit<Op::Jump>(Label { if_not_undefined_block });
 
             switch_to_basic_block(if_not_undefined_block);
@@ -98,23 +96,20 @@ CodeGenerationErrorOr<void> Generator::emit_function_declaration_instantiation(E
         if (auto const* identifier = parameter.binding.get_pointer<NonnullRefPtr<Identifier const>>(); identifier) {
             if ((*identifier)->is_local()) {
                 auto local_variable_index = (*identifier)->local_variable_index();
-                emit<Op::GetArgument>(local(local_variable_index), param_index);
+                emit<Op::Mov>(local(local_variable_index), Operand { Operand::Type::Argument, param_index });
                 set_local_initialized((*identifier)->local_variable_index());
             } else {
                 auto id = intern_identifier((*identifier)->string());
-                auto argument_reg = allocate_register();
-                emit<Op::GetArgument>(argument_reg.operand(), param_index);
                 if (function.shared_data().m_has_duplicates) {
-                    emit<Op::SetLexicalBinding>(id, argument_reg.operand());
+                    emit<Op::SetLexicalBinding>(id, Operand { Operand::Type::Argument, param_index });
                 } else {
-                    emit<Op::InitializeLexicalBinding>(id, argument_reg.operand());
+                    emit<Op::InitializeLexicalBinding>(id, Operand { Operand::Type::Argument, param_index });
                 }
             }
         } else if (auto const* binding_pattern = parameter.binding.get_pointer<NonnullRefPtr<BindingPattern const>>(); binding_pattern) {
-            auto input_operand = allocate_register();
-            emit<Op::GetArgument>(input_operand.operand(), param_index);
+            ScopedOperand argument { *this, Operand { Operand::Type::Argument, param_index } };
             auto init_mode = function.shared_data().m_has_duplicates ? Op::BindingInitializationMode::Set : Bytecode::Op::BindingInitializationMode::Initialize;
-            TRY((*binding_pattern)->generate_bytecode(*this, init_mode, input_operand));
+            TRY((*binding_pattern)->generate_bytecode(*this, init_mode, argument));
         }
     }
 
@@ -312,6 +307,7 @@ CodeGenerationErrorOr<GC::Ref<Executable>> Generator::compile(VM& vm, ASTNode co
 
     auto number_of_registers = generator.m_next_register;
     auto number_of_constants = generator.m_constants.size();
+    auto number_of_locals = function ? function->local_variables_names().size() : 0;
 
     // Pass: Rewrite the bytecode to use the correct register and constant indices.
     for (auto& block : generator.m_root_basic_blocks) {
@@ -319,7 +315,7 @@ CodeGenerationErrorOr<GC::Ref<Executable>> Generator::compile(VM& vm, ASTNode co
         while (!it.at_end()) {
             auto& instruction = const_cast<Instruction&>(*it);
 
-            instruction.visit_operands([number_of_registers, number_of_constants](Operand& operand) {
+            instruction.visit_operands([number_of_registers, number_of_constants, number_of_locals](Operand& operand) {
                 switch (operand.type()) {
                 case Operand::Type::Register:
                     break;
@@ -329,6 +325,9 @@ CodeGenerationErrorOr<GC::Ref<Executable>> Generator::compile(VM& vm, ASTNode co
                 case Operand::Type::Constant:
                     operand.offset_index_by(number_of_registers);
                     break;
+                case Operand::Type::Argument:
+                    operand.offset_index_by(number_of_registers + number_of_constants + number_of_locals);
+                    break;
                 default:
                     VERIFY_NOT_REACHED();
                 }
@@ -476,6 +475,7 @@ CodeGenerationErrorOr<GC::Ref<Executable>> Generator::compile(VM& vm, ASTNode co
     executable->source_map = move(source_map);
     executable->local_variable_names = move(local_variable_names);
     executable->local_index_base = number_of_registers + number_of_constants;
+    executable->argument_index_base = number_of_registers + number_of_constants + number_of_locals;
     executable->length_identifier = generator.m_length_identifier;
 
     generator.m_finished = true;
diff --git a/Libraries/LibJS/Bytecode/Instruction.h b/Libraries/LibJS/Bytecode/Instruction.h
index 00ea60b96b5..0b81a8fd93d 100644
--- a/Libraries/LibJS/Bytecode/Instruction.h
+++ b/Libraries/LibJS/Bytecode/Instruction.h
@@ -51,7 +51,6 @@
     O(EnterObjectEnvironment)          \
     O(EnterUnwindContext)              \
     O(Exp)                             \
-    O(GetArgument)                     \
     O(GetById)                         \
     O(GetByIdWithThis)                 \
     O(GetByValue)                      \
@@ -131,7 +130,6 @@
     O(Return)                          \
     O(RightShift)                      \
     O(ScheduleJump)                    \
-    O(SetArgument)                     \
     O(SetCompletionType)               \
     O(SetLexicalBinding)               \
     O(SetVariableBinding)              \
diff --git a/Libraries/LibJS/Bytecode/Interpreter.cpp b/Libraries/LibJS/Bytecode/Interpreter.cpp
index 358762db20e..4f41a4c8aa2 100644
--- a/Libraries/LibJS/Bytecode/Interpreter.cpp
+++ b/Libraries/LibJS/Bytecode/Interpreter.cpp
@@ -83,6 +83,9 @@ static ByteString format_operand(StringView name, Operand operand, Bytecode::Exe
     case Operand::Type::Local:
         builder.appendff("\033[34m{}~{}\033[0m", executable.local_variable_names[operand.index() - executable.local_index_base], operand.index() - executable.local_index_base);
         break;
+    case Operand::Type::Argument:
+        builder.appendff("\033[34marg{}\033[0m", operand.index() - executable.argument_index_base);
+        break;
     case Operand::Type::Constant: {
         builder.append("\033[36m"sv);
         auto value = executable.constants[operand.index() - executable.number_of_registers];
@@ -184,12 +187,12 @@ Interpreter::~Interpreter()
 
 ALWAYS_INLINE Value Interpreter::get(Operand op) const
 {
-    return m_registers_and_constants_and_locals.data()[op.index()];
+    return m_registers_and_constants_and_locals_arguments.data()[op.index()];
 }
 
 ALWAYS_INLINE void Interpreter::set(Operand op, Value value)
 {
-    m_registers_and_constants_and_locals.data()[op.index()] = value;
+    m_registers_and_constants_and_locals_arguments.data()[op.index()] = value;
 }
 
 ALWAYS_INLINE Value Interpreter::do_yield(Value value, Optional<Label> continuation)
@@ -374,7 +377,6 @@ FLATTEN_ON_CLANG void Interpreter::run_bytecode(size_t entry_point)
     }
 
     auto& running_execution_context = this->running_execution_context();
-    auto* arguments = running_execution_context.arguments.data();
     auto& accumulator = this->accumulator();
     auto& executable = current_executable();
     auto const* bytecode = executable.bytecode.data();
@@ -408,18 +410,6 @@ FLATTEN_ON_CLANG void Interpreter::run_bytecode(size_t entry_point)
         for (;;) {
             goto* bytecode_dispatch_table[static_cast<size_t>((*reinterpret_cast<Instruction const*>(&bytecode[program_counter])).type())];
 
-        handle_GetArgument: {
-            auto const& instruction = *reinterpret_cast<Op::GetArgument const*>(&bytecode[program_counter]);
-            set(instruction.dst(), arguments[instruction.index()]);
-            DISPATCH_NEXT(GetArgument);
-        }
-
-        handle_SetArgument: {
-            auto const& instruction = *reinterpret_cast<Op::SetArgument const*>(&bytecode[program_counter]);
-            arguments[instruction.index()] = get(instruction.src());
-            DISPATCH_NEXT(SetArgument);
-        }
-
         handle_Mov: {
             auto& instruction = *reinterpret_cast<Op::Mov const*>(&bytecode[program_counter]);
             set(instruction.dst(), get(instruction.src()));
@@ -745,7 +735,7 @@ Interpreter::ResultAndReturnRegister Interpreter::run_executable(Executable& exe
     VERIFY(registers_and_constants_and_locals_count <= running_execution_context.registers_and_constants_and_locals_and_arguments_span().size());
 
     TemporaryChange restore_running_execution_context { m_running_execution_context, &running_execution_context };
-    TemporaryChange restore_registers_and_constants_and_locals { m_registers_and_constants_and_locals, running_execution_context.registers_and_constants_and_locals_and_arguments_span() };
+    TemporaryChange restore_registers_and_constants_and_locals { m_registers_and_constants_and_locals_arguments, running_execution_context.registers_and_constants_and_locals_and_arguments_span() };
 
     reg(Register::accumulator()) = initial_accumulator_value;
     reg(Register::return_value()) = js_special_empty_value();
@@ -3219,16 +3209,6 @@ ByteString SetVariableBinding::to_byte_string_impl(Bytecode::Executable const& e
         format_operand("src"sv, src(), executable));
 }
 
-ByteString GetArgument::to_byte_string_impl(Bytecode::Executable const& executable) const
-{
-    return ByteString::formatted("GetArgument {}, {}", index(), format_operand("dst"sv, dst(), executable));
-}
-
-ByteString SetArgument::to_byte_string_impl(Bytecode::Executable const& executable) const
-{
-    return ByteString::formatted("SetArgument {}, {}", index(), format_operand("src"sv, src(), executable));
-}
-
 static StringView property_kind_to_string(PropertyKind kind)
 {
     switch (kind) {
diff --git a/Libraries/LibJS/Bytecode/Interpreter.h b/Libraries/LibJS/Bytecode/Interpreter.h
index b6b009a29c7..5278f55898e 100644
--- a/Libraries/LibJS/Bytecode/Interpreter.h
+++ b/Libraries/LibJS/Bytecode/Interpreter.h
@@ -49,11 +49,11 @@ public:
     ALWAYS_INLINE Value& saved_return_value() { return reg(Register::saved_return_value()); }
     Value& reg(Register const& r)
     {
-        return m_registers_and_constants_and_locals.data()[r.index()];
+        return m_registers_and_constants_and_locals_arguments.data()[r.index()];
     }
     Value reg(Register const& r) const
     {
-        return m_registers_and_constants_and_locals.data()[r.index()];
+        return m_registers_and_constants_and_locals_arguments.data()[r.index()];
     }
 
     [[nodiscard]] Value get(Operand) const;
@@ -101,7 +101,7 @@ private:
     GC::Ptr<Object> m_global_object { nullptr };
     GC::Ptr<DeclarativeEnvironment> m_global_declarative_environment { nullptr };
     Optional<size_t&> m_program_counter;
-    Span<Value> m_registers_and_constants_and_locals;
+    Span<Value> m_registers_and_constants_and_locals_arguments;
     Vector<Value> m_argument_values_buffer;
     ExecutionContext* m_running_execution_context { nullptr };
 };
diff --git a/Libraries/LibJS/Bytecode/Op.h b/Libraries/LibJS/Bytecode/Op.h
index 83bdeeecbf1..95151084545 100644
--- a/Libraries/LibJS/Bytecode/Op.h
+++ b/Libraries/LibJS/Bytecode/Op.h
@@ -775,52 +775,6 @@ private:
     mutable EnvironmentCoordinate m_cache;
 };
 
-class SetArgument final : public Instruction {
-public:
-    SetArgument(size_t index, Operand src)
-        : Instruction(Type::SetArgument)
-        , m_index(index)
-        , m_src(src)
-    {
-    }
-
-    ByteString to_byte_string_impl(Bytecode::Executable const&) const;
-    void visit_operands_impl(Function<void(Operand&)> visitor)
-    {
-        visitor(m_src);
-    }
-
-    size_t index() const { return m_index; }
-    Operand src() const { return m_src; }
-
-private:
-    u32 m_index;
-    Operand m_src;
-};
-
-class GetArgument final : public Instruction {
-public:
-    GetArgument(Operand dst, size_t index)
-        : Instruction(Type::GetArgument)
-        , m_index(index)
-        , m_dst(dst)
-    {
-    }
-
-    ByteString to_byte_string_impl(Bytecode::Executable const&) const;
-    void visit_operands_impl(Function<void(Operand&)> visitor)
-    {
-        visitor(m_dst);
-    }
-
-    u32 index() const { return m_index; }
-    Operand dst() const { return m_dst; }
-
-private:
-    u32 m_index;
-    Operand m_dst;
-};
-
 class GetCalleeAndThisFromEnvironment final : public Instruction {
 public:
     explicit GetCalleeAndThisFromEnvironment(Operand callee, Operand this_value, IdentifierTableIndex identifier)
diff --git a/Libraries/LibJS/Bytecode/Operand.h b/Libraries/LibJS/Bytecode/Operand.h
index b5052f19012..874a046f1a3 100644
--- a/Libraries/LibJS/Bytecode/Operand.h
+++ b/Libraries/LibJS/Bytecode/Operand.h
@@ -22,6 +22,7 @@ public:
         Register,
         Local,
         Constant,
+        Argument,
     };
 
     [[nodiscard]] bool operator==(Operand const&) const = default;
@@ -53,8 +54,8 @@ private:
     //       Because this type is absolutely essential to the interpreter, we allow
     //       ourselves this little ifdef.
 #if ARCH(AARCH64)
-    Type m_type : 2 {};
-    u32 m_index : 30 { 0 };
+    Type m_type : 3 {};
+    u32 m_index : 29 { 0 };
 #else
     Type m_type { Type::Invalid };
     u32 m_index { 0 };