Skip to content

Commit

Permalink
[vm/bytecode] Revise PushStatic bytecode instruction
Browse files Browse the repository at this point in the history 
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 <alexmarkov@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Régis Crelier <regis@google.com>
  • Loading branch information
@alexmarkov
alexmarkov authored and commit-bot@chromium.org committed Aug 9, 2019
1 parent f74b0cc commit f255c0c
Show file tree
Hide file tree
Showing 9 changed files with 62 additions and 48 deletions.
8 changes: 4 additions & 4 deletions pkg/vm/lib/bytecode/assembler.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
13 changes: 6 additions & 7 deletions pkg/vm/lib/bytecode/dbc.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -168,8 +168,8 @@ enum Opcode {
kUnused17, // Reserved for PushParamLast1
kPopLocal,
kPopLocal_Wide,
kUnused18, // Reserved for PopLocal0
kUnused19,
kLoadStatic,
kLoadStatic_Wide,
kStoreLocal,
kStoreLocal_Wide,

Expand All @@ -182,8 +182,8 @@ enum Opcode {
kUnused20,

// Static fields.
kPushStatic,
kPushStatic_Wide,
kUnused40,
kUnused41,
kStoreStaticTOS,
kStoreStaticTOS_Wide,

Expand Down Expand Up @@ -412,7 +412,7 @@ const Map<Opcode, Format> 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]),
Expand Down Expand Up @@ -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;
Expand Down
5 changes: 1 addition & 4 deletions pkg/vm/lib/bytecode/gen_bytecode.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3074,10 +3074,7 @@ class BytecodeGenerator extends RecursiveVisitor<Null> {
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);
}
Expand Down
42 changes: 14 additions & 28 deletions pkg/vm/testcases/bytecode/bootstrapping.dart.expect
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -232,8 +225,7 @@ L1:
L2:
Push r0
Drop1
PushConstant CP#0
PushStatic CP#0
LoadStatic CP#0
ReturnTOS
}
ConstantPool {
Expand Down Expand Up @@ -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
Expand All @@ -403,8 +394,7 @@ Bytecode {
DirectCall CP#6, 2
StoreStaticTOS CP#0
L1:
PushConstant CP#0
PushStatic CP#0
LoadStatic CP#0
ReturnTOS
}
ConstantPool {
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions pkg/vm/testcases/bytecode/literals.dart.expect
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions runtime/vm/compiler/assembler/disassembler_kbc.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
19 changes: 19 additions & 0 deletions runtime/vm/compiler/frontend/bytecode_flow_graph_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions runtime/vm/constants_kbc.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, ___, ___) \
Expand Down Expand Up @@ -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,
Expand Down
12 changes: 12 additions & 0 deletions runtime/vm/interpreter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2362,13 +2362,25 @@ RawObject* Interpreter::Call(RawFunction* function,
}

{
static_assert(KernelBytecode::kMinSupportedBytecodeFormatVersion < 19,
"Cleanup PushStatic bytecode instruction");
BYTECODE(PushStatic, D);
RawField* field = reinterpret_cast<RawField*>(LOAD_CONSTANT(rD));
// Note: field is also on the stack, hence no increment.
*SP = field->ptr()->value_.static_value_;
DISPATCH();
}

{
BYTECODE(LoadStatic, D);
RawField* field = reinterpret_cast<RawField*>(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));
Expand Down

0 comments on commit f255c0c

Please sign in to comment.