Skip to content

Commit e301d71

Browse files
GeorgNeisV8 LUCI CQ
GeorgNeis
authored and
V8 LUCI CQ
committed
[compiler] Teach InstructionScheduler about protected memory accesses
Because these instructions can trap, we don't want them to be reordered as freely as unprotected accesses. As part of this, make explicit which opcodes support a MemoryAccessMode. Bug: v8:12018 Change-Id: I9db3053d7d62ffce6d3c95d62adce71ae40dae62 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3172770 Reviewed-by: Clemens Backes <clemensb@chromium.org> Commit-Queue: Georg Neis <neis@chromium.org> Cr-Commit-Position: refs/heads/main@{#77031}
1 parent bb5fa03 commit e301d71

10 files changed

Lines changed: 1483 additions & 1430 deletions

‎src/compiler/backend/arm/instruction-codes-arm.h‎

Lines changed: 356 additions & 351 deletions
Large diffs are not rendered by default.

‎src/compiler/backend/arm64/instruction-codes-arm64.h‎

Lines changed: 338 additions & 333 deletions
Large diffs are not rendered by default.

‎src/compiler/backend/ia32/instruction-codes-ia32.h‎

Lines changed: 353 additions & 348 deletions
Large diffs are not rendered by default.

‎src/compiler/backend/instruction-codes.h‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -296,23 +296,50 @@ static_assert(
296296
"All addressing modes must fit in the 5-bit AddressingModeField.");
297297
using FlagsModeField = base::BitField<FlagsMode, 14, 3>;
298298
using FlagsConditionField = base::BitField<FlagsCondition, 17, 5>;
299-
using DeoptImmedArgsCountField = base::BitField<int, 22, 2>;
300-
using DeoptFrameStateOffsetField = base::BitField<int, 24, 8>;
299+
using MiscField = base::BitField<int, 22, 10>;
300+
301+
// {MiscField} is used for a variety of things, depending on the opcode.
302+
// TODO(turbofan): There should be an abstraction that ensures safe encoding and
303+
// decoding. {HasMemoryAccessMode} and its uses are a small step in that
304+
// direction.
305+
301306
// LaneSizeField and AccessModeField are helper types to encode/decode a lane
302307
// size, an access mode, or both inside the overlapping MiscField.
303308
using LaneSizeField = base::BitField<int, 22, 8>;
304309
using AccessModeField = base::BitField<MemoryAccessMode, 30, 2>;
310+
// TODO(turbofan): {HasMemoryAccessMode} is currently only used to guard
311+
// decoding (in CodeGenerator and InstructionScheduler). Encoding (in
312+
// InstructionSelector) is not yet guarded. There are in fact instructions for
313+
// which InstructionSelector does set a MemoryAccessMode but CodeGenerator
314+
// doesn't care to consume it (e.g. kArm64LdrDecompressTaggedSigned). This is
315+
// scary. {HasMemoryAccessMode} does not include these instructions, so they can
316+
// be easily found by guarding encoding.
317+
inline bool HasMemoryAccessMode(ArchOpcode opcode) {
318+
switch (opcode) {
319+
#define CASE(Name) \
320+
case k##Name: \
321+
return true;
322+
TARGET_ARCH_OPCODE_WITH_MEMORY_ACCESS_MODE_LIST(CASE)
323+
#undef CASE
324+
default:
325+
return false;
326+
}
327+
}
328+
329+
using DeoptImmedArgsCountField = base::BitField<int, 22, 2>;
330+
using DeoptFrameStateOffsetField = base::BitField<int, 24, 8>;
331+
305332
// AtomicWidthField overlaps with MiscField and is used for the various Atomic
306333
// opcodes. Only used on 64bit architectures. All atomic instructions on 32bit
307334
// architectures are assumed to be 32bit wide.
308335
using AtomicWidthField = base::BitField<AtomicWidth, 22, 2>;
336+
309337
// AtomicMemoryOrderField overlaps with MiscField and is used for the various
310338
// Atomic opcodes. This field is not used on all architectures. It is used on
311339
// architectures where the codegen for kSeqCst and kAcqRel differ only by
312340
// emitting fences.
313341
using AtomicMemoryOrderField = base::BitField<AtomicMemoryOrder, 24, 2>;
314342
using AtomicStoreRecordWriteModeField = base::BitField<RecordWriteMode, 26, 4>;
315-
using MiscField = base::BitField<int, 22, 10>;
316343

317344
// This static assertion serves as an early warning if we are about to exhaust
318345
// the available opcode space. If we are about to exhaust it, we should start

‎src/compiler/backend/instruction-scheduler.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ void InstructionScheduler::AddInstruction(Instruction* instr) {
167167
last_side_effect_instr_->AddSuccessor(new_node);
168168
}
169169
pending_loads_.push_back(new_node);
170-
} else if (instr->IsDeoptimizeCall() || instr->IsTrap()) {
170+
} else if (instr->IsDeoptimizeCall() || CanTrap(instr)) {
171171
// Ensure that deopts or traps are not reordered with respect to
172172
// side-effect instructions.
173173
if (last_side_effect_instr_ != nullptr) {
@@ -176,7 +176,7 @@ void InstructionScheduler::AddInstruction(Instruction* instr) {
176176
}
177177

178178
// Update last deoptimization or trap point.
179-
if (instr->IsDeoptimizeCall() || instr->IsTrap()) {
179+
if (instr->IsDeoptimizeCall() || CanTrap(instr)) {
180180
last_deopt_or_trap_ = new_node;
181181
}
182182

‎src/compiler/backend/instruction-scheduler.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,12 @@ class InstructionScheduler final : public ZoneObject {
169169
return (GetInstructionFlags(instr) & kIsLoadOperation) != 0;
170170
}
171171

172+
bool CanTrap(const Instruction* instr) const {
173+
return instr->IsTrap() ||
174+
(instr->HasMemoryAccessMode() &&
175+
instr->memory_access_mode() == kMemoryAccessProtected);
176+
}
177+
172178
// The scheduler will not move the following instructions before the last
173179
// deopt/trap check:
174180
// * loads (this is conservative)
@@ -184,7 +190,7 @@ class InstructionScheduler final : public ZoneObject {
184190
// trap point we encountered.
185191
bool DependsOnDeoptOrTrap(const Instruction* instr) const {
186192
return MayNeedDeoptOrTrapCheck(instr) || instr->IsDeoptimizeCall() ||
187-
instr->IsTrap() || HasSideEffect(instr) || IsLoadOperation(instr);
193+
CanTrap(instr) || HasSideEffect(instr) || IsLoadOperation(instr);
188194
}
189195

190196
// Identify nops used as a definition point for live-in registers at

‎src/compiler/backend/instruction.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -882,6 +882,13 @@ class V8_EXPORT_PRIVATE Instruction final {
882882
return FlagsConditionField::decode(opcode());
883883
}
884884
int misc() const { return MiscField::decode(opcode()); }
885+
bool HasMemoryAccessMode() const {
886+
return compiler::HasMemoryAccessMode(arch_opcode());
887+
}
888+
MemoryAccessMode memory_access_mode() const {
889+
DCHECK(HasMemoryAccessMode());
890+
return AccessModeField::decode(opcode());
891+
}
885892

886893
static Instruction* New(Zone* zone, InstructionCode opcode) {
887894
return New(zone, opcode, 0, nullptr, 0, nullptr, 0, nullptr);

‎src/compiler/backend/x64/code-generator-x64.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -693,7 +693,7 @@ class WasmProtectedInstructionTrap final : public WasmOutOfLineTrap {
693693
void EmitOOLTrapIfNeeded(Zone* zone, CodeGenerator* codegen,
694694
InstructionCode opcode, Instruction* instr,
695695
int pc) {
696-
const MemoryAccessMode access_mode = AccessModeField::decode(opcode);
696+
const MemoryAccessMode access_mode = instr->memory_access_mode();
697697
if (access_mode == kMemoryAccessProtected) {
698698
zone->New<WasmProtectedInstructionTrap>(codegen, pc, instr);
699699
}
@@ -703,7 +703,7 @@ void EmitOOLTrapIfNeeded(Zone* zone, CodeGenerator* codegen,
703703

704704
void EmitOOLTrapIfNeeded(Zone* zone, CodeGenerator* codegen,
705705
InstructionCode opcode, Instruction* instr, int pc) {
706-
DCHECK_NE(kMemoryAccessProtected, AccessModeField::decode(opcode));
706+
DCHECK_NE(kMemoryAccessProtected, instr->memory_access_mode());
707707
}
708708

709709
#endif // V8_ENABLE_WEBASSEMBLY

0 commit comments

Comments
 (0)