Skip to content

Commit a497308

Browse files
danlehV8 LUCI CQ
danleh
authored and
V8 LUCI CQ
committed
[wasm] Fix deopt loop with cross-instance call_indirect
See discussion in https://crrev.com/c/6011089 for context. Bug: 335082212, 377744184 Change-Id: I52ce0ff2636650a644cc4daa6b606a7b2172a826 Fixed: 378584789 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6011807 Commit-Queue: Daniel Lehmann <[email protected]> Reviewed-by: Matthias Liedtke <[email protected]> Cr-Commit-Position: refs/heads/main@{#97149}
1 parent 897e8ec commit a497308

File tree

3 files changed

+40
-35
lines changed

3 files changed

+40
-35
lines changed

‎src/builtins/wasm.tq‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -708,7 +708,9 @@ macro UpdateCallRefOrIndirectIC(
708708
// The "ic::IsMegamorphic(firstSlot)" case doesn't need to do anything.
709709
} else {
710710
// Monomorphic miss.
711-
dcheck(Is<WasmFuncRef>(firstSlot) || Is<Smi>(firstSlot));
711+
dcheck(
712+
Is<WasmFuncRef>(firstSlot) || Is<Smi>(firstSlot) ||
713+
IsUndefined(firstSlot));
712714
const newEntries = UnsafeCast<FixedArray>(AllocateFixedArray(
713715
ElementsKind::PACKED_ELEMENTS, 4, AllocationFlag::kNone));
714716
newEntries.objects[0] = firstSlot;
@@ -753,19 +755,15 @@ builtin CallRefIC(
753755
builtin CallIndirectIC(
754756
vector: FixedArray, vectorIndex: int32, target: WasmCodePointer,
755757
implicitArg: WasmTrustedInstanceData|WasmImportData): TargetAndImplicitArg {
756-
// We increase the call counter for the target, regardless of whether the
757-
// instance (in `implicitArg`) matches the current instance or not.
758-
// Technically this overcounts feedback of cross-instance, same module
759-
// functions, but in practice this is rare enough to not be worth the added
760-
// branch here.
761-
// Background: For correctness, we can only execute the inlined function's
762-
// code if the instance of the (pre-inlining) call target is equal to the
763-
// instance of the currently executing caller, see Turboshaft codegen.
764-
765-
const truncatedTarget =
766-
SmiTag(Signed(Convert<uintptr>(target) & kSmiMaxValue));
758+
// If this is a cross-instance call, don't track the precise target (we can
759+
// only correctly inline same-instance calls anyway). Mark it as `Undefined`,
760+
// so that we can prevent a deopt loop, see `TransitiveTypeFeedbackProcessor`.
761+
const instance = LoadInstanceDataFromFrame();
762+
const truncatedTargetOrCrossInstance = TaggedEqual(implicitArg, instance) ?
763+
SmiTag(Signed(Convert<uintptr>(target) & kSmiMaxValue)) :
764+
Undefined;
767765
UpdateCallRefOrIndirectIC(
768-
vector, Convert<intptr>(vectorIndex), truncatedTarget);
766+
vector, Convert<intptr>(vectorIndex), truncatedTargetOrCrossInstance);
769767

770768
return TargetAndImplicitArg{target: target, implicit_arg: implicitArg};
771769
}

‎src/wasm/module-compiler.cc‎

Lines changed: 29 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1355,7 +1355,7 @@ class FeedbackMaker {
13551355
void AddCallRefCandidate(Tagged<WasmFuncRef> funcref, int count) {
13561356
Tagged<WasmInternalFunction> internal_function =
13571357
Cast<WasmFuncRef>(funcref)->internal(isolate_);
1358-
// Only consider wasm function declared in this instance.
1358+
// Discard cross-instance calls, as we can only inline same-instance code.
13591359
if (internal_function->implicit_arg() != instance_data_) {
13601360
has_non_inlineable_targets_ = true;
13611361
return;
@@ -1368,7 +1368,16 @@ class FeedbackMaker {
13681368
AddCall(internal_function->function_index(), count);
13691369
}
13701370

1371-
void AddCallIndirectCandidate(Tagged<Smi> target_truncated_smi, int count) {
1371+
void AddCallIndirectCandidate(Tagged<Object> target_truncated_obj,
1372+
int count) {
1373+
// Discard cross-instance calls, as we can only inline same-instance code.
1374+
bool is_cross_instance_call = IsUndefined(target_truncated_obj);
1375+
if (is_cross_instance_call) {
1376+
has_non_inlineable_targets_ = true;
1377+
return;
1378+
}
1379+
Tagged<Smi> target_truncated_smi = Cast<Smi>(target_truncated_obj);
1380+
13721381
// We need to map a truncated call target back to a function index.
13731382
// Generally there may be multiple jump tables if code spaces are far apart
13741383
// (to ensure that direct calls can always use a near call to the closest
@@ -1494,21 +1503,22 @@ void TransitiveTypeFeedbackProcessor::ProcessFunction(int func_index) {
14941503

14951504
// For each entry in {call_targets}, there are two {Object} slots in the
14961505
// {feedback} vector:
1506+
// +--------------------------+-----------------------+-------------------+
1507+
// | Call Type | Feedback: Entry 1 | Entry 2 |
14971508
// +-------------------------+-----------------------+-------------------+
1498-
// | Call Type | Feedback: Entry 1 | Entry 2 |
1499-
// +-------------------------+-----------------------+-------------------+
1500-
// | direct | Smi(count) | Smi(0), unused |
1501-
// +-------------------------+-----------------------+-------------------+
1502-
// | ref, uninitialized | Smi(0) | Smi(0) |
1503-
// | ref, monomorphic | WasmFuncRef(target) | Smi(count>0) |
1504-
// | ref, polymorphic | FixedArray | Undefined |
1505-
// | ref, megamorphic | MegamorphicSymbol | Undefined |
1506-
// +-------------------------+-----------------------+-------------------+
1507-
// | indirect, uninitialized | Smi(0) | Smi(0) |
1508-
// | indirect, monomorphic | Smi(truncated_target) | Smi(count>0) |
1509-
// | indirect, polymorphic | FixedArray | Undefined |
1510-
// | indirect, megamorphic | MegamorphicSymbol | Undefined |
1511-
// +-------------------------+-----------------------+-------------------+
1509+
// | direct | Smi(count) | Smi(0), unused |
1510+
// +--------------------------+-----------------------+-------------------+
1511+
// | ref, uninitialized | Smi(0) | Smi(0) |
1512+
// | ref, monomorphic | WasmFuncRef(target) | Smi(count>0) |
1513+
// | ref, polymorphic | FixedArray | Undefined |
1514+
// | ref, megamorphic | MegamorphicSymbol | Undefined |
1515+
// +--------------------------+-----------------------+-------------------+
1516+
// | indirect, uninitialized | Smi(0) | Smi(0) |
1517+
// | indirect, monomorphic | Smi(truncated_target) | Smi(count>0) |
1518+
// | indirect, wrong instance | Undefined | Smi(count>0) |
1519+
// | indirect, polymorphic | FixedArray | Undefined |
1520+
// | indirect, megamorphic | MegamorphicSymbol | Undefined |
1521+
// +--------------------------+-----------------------+-------------------+
15121522
// The FixedArray entries for the polymorphic cases look like the monomorphic
15131523
// entries in the feedback vector itself.
15141524
// See {UpdateCallRefOrIndirectIC} in {wasm.tq} for how this is written.
@@ -1543,11 +1553,11 @@ void TransitiveTypeFeedbackProcessor::ProcessFunction(int func_index) {
15431553
DCHECK_EQ(sentinel_or_target, FunctionTypeFeedback::kCallRef);
15441554
int count = Smi::ToInt(second_slot);
15451555
fm.AddCallRefCandidate(Cast<WasmFuncRef>(first_slot), count);
1546-
} else if (IsSmi(first_slot)) {
1556+
} else if (IsSmi(first_slot) || IsUndefined(first_slot)) {
15471557
// Monomorphic call_indirect.
15481558
DCHECK_EQ(sentinel_or_target, FunctionTypeFeedback::kCallIndirect);
15491559
int count = Smi::ToInt(second_slot);
1550-
fm.AddCallIndirectCandidate(Cast<Smi>(first_slot), count);
1560+
fm.AddCallIndirectCandidate(first_slot, count);
15511561
} else if (IsFixedArray(first_slot)) {
15521562
// Polymorphic call_ref or call_indirect.
15531563
Tagged<FixedArray> polymorphic = Cast<FixedArray>(first_slot);
@@ -1563,7 +1573,7 @@ void TransitiveTypeFeedbackProcessor::ProcessFunction(int func_index) {
15631573
} else {
15641574
DCHECK_EQ(sentinel_or_target, FunctionTypeFeedback::kCallIndirect);
15651575
for (int j = 0; j < checked_polymorphic_length; j += 2) {
1566-
Tagged<Smi> target = Cast<Smi>(polymorphic->get(j));
1576+
Tagged<Object> target = polymorphic->get(j);
15671577
int count = Smi::ToInt(polymorphic->get(j + 1));
15681578
fm.AddCallIndirectCandidate(target, count);
15691579
}

‎test/mjsunit/mjsunit.status‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,6 @@
8686
'regress/proto-transition-regress': [SKIP],
8787
'regress/regress-357651585': [SKIP],
8888

89-
# https://crbug.com/378584789: deopt loop with cross-instance call_indirect
90-
'wasm/deopt/deopt-multi-instance-call-indirect': [SKIP],
91-
9289
##############################################################################
9390
# Tests where variants make no sense.
9491
'd8/enable-tracing': [PASS, NO_VARIANTS],

0 commit comments

Comments
 (0)