From f255c0c8d3a0f262e5ee18cadc0707b70285177a Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Fri, 9 Aug 2019 21:02:31 +0000 Subject: [PATCH] [vm/bytecode] Revise PushStatic bytecode instruction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PushStatic instruction expects field object to be pushed onto the stack. This is useless, as PushStatic also references field with its D operand. This change replaces PushStatic instruction with LoadStatic which doesn't take field object on the stack, so PushConstant/PushStatic pair is replaced with a single LoadStatic instruction. This change also enables constant propagation of values of injected CID fields, which are not known at bytecode generation time. Change-Id: Ifbdd3aea2aab338f6c9ec3e5728948c4d8541a4b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/112489 Commit-Queue: Alexander Markov Reviewed-by: Ryan Macnak Reviewed-by: Régis Crelier --- pkg/vm/lib/bytecode/assembler.dart | 8 ++-- pkg/vm/lib/bytecode/dbc.dart | 13 +++--- pkg/vm/lib/bytecode/gen_bytecode.dart | 5 +-- .../bytecode/bootstrapping.dart.expect | 42 +++++++------------ .../testcases/bytecode/literals.dart.expect | 3 +- .../vm/compiler/assembler/disassembler_kbc.cc | 2 + .../frontend/bytecode_flow_graph_builder.cc | 19 +++++++++ runtime/vm/constants_kbc.h | 6 +-- runtime/vm/interpreter.cc | 12 ++++++ 9 files changed, 62 insertions(+), 48 deletions(-) diff --git a/pkg/vm/lib/bytecode/assembler.dart b/pkg/vm/lib/bytecode/assembler.dart index 1fe11b49377c..c28783898ce9 100644 --- a/pkg/vm/lib/bytecode/assembler.dart +++ b/pkg/vm/lib/bytecode/assembler.dart @@ -404,15 +404,15 @@ class BytecodeAssembler { _emitInstructionD(Opcode.kNativeCall, rd); } + void emitLoadStatic(int rd) { + _emitInstructionD(Opcode.kLoadStatic, rd); + } + void emitStoreStaticTOS(int rd) { emitSourcePosition(); _emitInstructionD(Opcode.kStoreStaticTOS, rd); } - void emitPushStatic(int rd) { - _emitInstructionD(Opcode.kPushStatic, rd); - } - void emitCreateArrayTOS() { _emitInstruction0(Opcode.kCreateArrayTOS); } diff --git a/pkg/vm/lib/bytecode/dbc.dart b/pkg/vm/lib/bytecode/dbc.dart index 8aecbc4f2920..f091eabc7c44 100644 --- a/pkg/vm/lib/bytecode/dbc.dart +++ b/pkg/vm/lib/bytecode/dbc.dart @@ -10,7 +10,7 @@ library vm.bytecode.dbc; /// Before bumping current bytecode version format, make sure that /// all users have switched to a VM which is able to consume new /// version of bytecode. -const int currentBytecodeFormatVersion = 18; +const int currentBytecodeFormatVersion = 19; enum Opcode { kUnusedOpcode000, @@ -168,8 +168,8 @@ enum Opcode { kUnused17, // Reserved for PushParamLast1 kPopLocal, kPopLocal_Wide, - kUnused18, // Reserved for PopLocal0 - kUnused19, + kLoadStatic, + kLoadStatic_Wide, kStoreLocal, kStoreLocal_Wide, @@ -182,8 +182,8 @@ enum Opcode { kUnused20, // Static fields. - kPushStatic, - kPushStatic_Wide, + kUnused40, + kUnused41, kStoreStaticTOS, kStoreStaticTOS_Wide, @@ -412,7 +412,7 @@ const Map BytecodeFormats = const { Encoding.kD, const [Operand.lit, Operand.none, Operand.none]), Opcode.kStoreIndexedTOS: const Format( Encoding.k0, const [Operand.none, Operand.none, Operand.none]), - Opcode.kPushStatic: const Format( + Opcode.kLoadStatic: const Format( Encoding.kD, const [Operand.lit, Operand.none, Operand.none]), Opcode.kStoreStaticTOS: const Format( Encoding.kD, const [Operand.lit, Operand.none, Operand.none]), @@ -633,7 +633,6 @@ bool isPush(Opcode opcode) { case Opcode.kPushTrue: case Opcode.kPushFalse: case Opcode.kPushInt: - case Opcode.kPushStatic: return true; default: return false; diff --git a/pkg/vm/lib/bytecode/gen_bytecode.dart b/pkg/vm/lib/bytecode/gen_bytecode.dart index fdebdc122f54..141e78aed111 100644 --- a/pkg/vm/lib/bytecode/gen_bytecode.dart +++ b/pkg/vm/lib/bytecode/gen_bytecode.dart @@ -3074,10 +3074,7 @@ class BytecodeGenerator extends RecursiveVisitor { if (target.isConst) { _genPushConstExpr(target.initializer); } else if (_hasTrivialInitializer(target)) { - final fieldIndex = cp.addStaticField(target); - asm.emitPushConstant( - fieldIndex); // TODO(alexmarkov): do we really need this? - asm.emitPushStatic(fieldIndex); + asm.emitLoadStatic(cp.addStaticField(target)); } else { _genDirectCall(target, objectTable.getArgDescHandle(0), 0, isGet: true); } diff --git a/pkg/vm/testcases/bytecode/bootstrapping.dart.expect b/pkg/vm/testcases/bytecode/bootstrapping.dart.expect index 1fcf91242ac9..15d60a10a7e3 100644 --- a/pkg/vm/testcases/bytecode/bootstrapping.dart.expect +++ b/pkg/vm/testcases/bytecode/bootstrapping.dart.expect @@ -124,14 +124,12 @@ Function '_scriptUri', static, reflectable, debuggable Bytecode { Entry 2 CheckStack 0 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 PushConstant CP#1 InterfaceCall CP#2, 2 AssertBoolean 0 JumpIfTrue L1 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 PushConstant CP#4 InterfaceCall CP#2, 2 AssertBoolean 0 @@ -143,8 +141,7 @@ L1: L2: Push r1 JumpIfTrue L3 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 PushConstant CP#5 InterfaceCall CP#2, 2 AssertBoolean 0 @@ -156,15 +153,13 @@ L3: L4: Push r0 JumpIfFalse L5 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 DirectCall CP#6, 1 ReturnTOS L5: DirectCall CP#8, 0 PushNull - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 DirectCall CP#10, 2 InterfaceCall CP#12, 2 ReturnTOS @@ -214,12 +209,10 @@ Function 'get:stdin', getter, static, reflectable, debuggable Bytecode { Entry 2 CheckStack 0 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 EqualsNull JumpIfFalse L1 - PushConstant CP#1 - PushStatic CP#1 + LoadStatic CP#1 DirectCall CP#2, 1 StoreLocal r1 Push r1 @@ -232,8 +225,7 @@ L1: L2: Push r0 Drop1 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 ReturnTOS } ConstantPool { @@ -390,8 +382,7 @@ Function 'get:_namespace', getter, static, reflectable, debuggable Bytecode { Entry 2 CheckStack 0 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 EqualsNull JumpIfFalse L1 Allocate CP#1 @@ -403,8 +394,7 @@ Bytecode { DirectCall CP#6, 2 StoreStaticTOS CP#0 L1: - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 ReturnTOS } ConstantPool { @@ -592,12 +582,10 @@ Function 'get:platformScript', getter, static, reflectable, debuggable Bytecode { Entry 1 CheckStack 0 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 EqualsNull JumpIfFalse L1 - PushConstant CP#1 - PushStatic CP#1 + LoadStatic CP#1 EqualsNull BooleanNegateTOS PopLocal r0 @@ -608,13 +596,11 @@ L1: L2: Push r0 JumpIfFalse L3 - PushConstant CP#1 - PushStatic CP#1 + LoadStatic CP#1 DynamicCall CP#3, 1 StoreStaticTOS CP#0 L3: - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 ReturnTOS } ConstantPool { diff --git a/pkg/vm/testcases/bytecode/literals.dart.expect b/pkg/vm/testcases/bytecode/literals.dart.expect index bbfb3d30fa3a..5b63921cc3e0 100644 --- a/pkg/vm/testcases/bytecode/literals.dart.expect +++ b/pkg/vm/testcases/bytecode/literals.dart.expect @@ -390,8 +390,7 @@ Function 'testFieldWithDoubleLiteralInitializer', static, reflectable, debuggabl Bytecode { Entry 0 CheckStack 0 - PushConstant CP#0 - PushStatic CP#0 + LoadStatic CP#0 ReturnTOS } ConstantPool { diff --git a/runtime/vm/compiler/assembler/disassembler_kbc.cc b/runtime/vm/compiler/assembler/disassembler_kbc.cc index 839c6e46392f..d2f0229bbcf5 100644 --- a/runtime/vm/compiler/assembler/disassembler_kbc.cc +++ b/runtime/vm/compiler/assembler/disassembler_kbc.cc @@ -247,6 +247,8 @@ static intptr_t GetConstantPoolIndex(const KBCInstr* instr) { case KernelBytecode::kPushConstant_Wide: case KernelBytecode::kStoreStaticTOS: case KernelBytecode::kStoreStaticTOS_Wide: + case KernelBytecode::kLoadStatic: + case KernelBytecode::kLoadStatic_Wide: case KernelBytecode::kPushStatic: case KernelBytecode::kPushStatic_Wide: case KernelBytecode::kAllocate: diff --git a/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc b/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc index 6f7428264253..f0e7b05e803b 100644 --- a/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc +++ b/runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc @@ -1207,6 +1207,25 @@ void BytecodeFlowGraphBuilder::BuildStoreStaticTOS() { code_ += B->StoreStaticField(position_, field); } +void BytecodeFlowGraphBuilder::BuildLoadStatic() { + const Constant operand = ConstantAt(DecodeOperandD()); + const auto& field = Field::Cast(operand.value()); + // All constant expressions (including access to const fields) are evaluated + // in bytecode. However, values of injected cid fields are only available in + // the VM. In such case, evaluate const fields with known value here. + if (field.is_const() && !field.has_initializer()) { + const auto& value = Object::ZoneHandle(Z, field.StaticValue()); + ASSERT((value.raw() != Object::sentinel().raw()) && + (value.raw() != Object::transition_sentinel().raw())); + code_ += B->Constant(value); + return; + } + PushConstant(operand); + code_ += B->LoadStaticField(); +} + +static_assert(KernelBytecode::kMinSupportedBytecodeFormatVersion < 19, + "Cleanup PushStatic bytecode instruction"); void BytecodeFlowGraphBuilder::BuildPushStatic() { // Note: Field object is both pushed into the stack and // available in constant pool entry D. diff --git a/runtime/vm/constants_kbc.h b/runtime/vm/constants_kbc.h index 1f972f92f73c..9ade44764984 100644 --- a/runtime/vm/constants_kbc.h +++ b/runtime/vm/constants_kbc.h @@ -615,8 +615,8 @@ namespace dart { V(Unused17, 0, RESV, ___, ___, ___) \ V(PopLocal, X, ORDN, xeg, ___, ___) \ V(PopLocal_Wide, X, WIDE, xeg, ___, ___) \ - V(Unused18, 0, RESV, ___, ___, ___) \ - V(Unused19, 0, RESV, ___, ___, ___) \ + V(LoadStatic, D, ORDN, lit, ___, ___) \ + V(LoadStatic_Wide, D, WIDE, lit, ___, ___) \ V(StoreLocal, X, ORDN, xeg, ___, ___) \ V(StoreLocal_Wide, X, WIDE, xeg, ___, ___) \ V(LoadFieldTOS, D, ORDN, lit, ___, ___) \ @@ -749,7 +749,7 @@ class KernelBytecode { // Maximum bytecode format version supported by VM. // The range of supported versions should include version produced by bytecode // generator (currentBytecodeFormatVersion in pkg/vm/lib/bytecode/dbc.dart). - static const intptr_t kMaxSupportedBytecodeFormatVersion = 18; + static const intptr_t kMaxSupportedBytecodeFormatVersion = 19; enum Opcode { #define DECLARE_BYTECODE(name, encoding, kind, op1, op2, op3) k##name, diff --git a/runtime/vm/interpreter.cc b/runtime/vm/interpreter.cc index aafaf7d49ccc..05f05c58bc68 100644 --- a/runtime/vm/interpreter.cc +++ b/runtime/vm/interpreter.cc @@ -2362,6 +2362,8 @@ RawObject* Interpreter::Call(RawFunction* function, } { + static_assert(KernelBytecode::kMinSupportedBytecodeFormatVersion < 19, + "Cleanup PushStatic bytecode instruction"); BYTECODE(PushStatic, D); RawField* field = reinterpret_cast(LOAD_CONSTANT(rD)); // Note: field is also on the stack, hence no increment. @@ -2369,6 +2371,16 @@ RawObject* Interpreter::Call(RawFunction* function, DISPATCH(); } + { + BYTECODE(LoadStatic, D); + RawField* field = reinterpret_cast(LOAD_CONSTANT(rD)); + RawInstance* value = field->ptr()->value_.static_value_; + ASSERT((value != Object::sentinel().raw()) && + (value != Object::transition_sentinel().raw())); + *++SP = value; + DISPATCH(); + } + { BYTECODE(StoreFieldTOS, D); RawField* field = RAW_CAST(Field, LOAD_CONSTANT(rD + 1));