[compiler] Support skipped write barrier verification in Turbofan [v8/v8 : main]
0 views
Skip to first unread message
Dominik Inführ (Gerrit)
Sep 3, 2025, 11:01:55 AM (4 days ago) Sep 3
to Darius Mercadier, AyeAye, V8 LUCI CQ, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Darius Mercadier
Dominik Inführ added 1 comment![]( "Open in Gerrit")
File src/compiler/turboshaft/memory-optimization-reducer.h
Line 227, Patchset 14:
WriteBarrierKind::kSkippedWriteBarrier, store.offset,Darius Mercadier . resolved
Dominik InführWhat about
```
v8_flags.verify_write_barriers ? SkippedWB : NoWB
```
?
Done
Related details
Attention is currently required from:
- Darius Mercadier
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Sep 4, 2025, 2:30:46 AM (3 days ago) Sep 4
to Darius Mercadier, AyeAye, V8 LUCI CQ, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Darius Mercadier
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Darius Mercadier
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Sep 4, 2025, 2:59:48 AM (3 days ago) Sep 4
to Dominik Inführ, AyeAye, V8 LUCI CQ, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ
Darius Mercadier voted and added 7 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
7 comments
Patchset-level comments
Darius Mercadier . resolved
LGTM % comments
File src/compiler/backend/arm64/code-generator-arm64.cc
Line 1399, Patchset 25 (Latest):
case kArchStoreIndirectNoWriteBarrier: {
AddressingMode addressing_mode =
AddressingModeField::decode(instr->opcode());
Register object = i.InputRegister(0);
Operand offset(0);
if (addressing_mode == kMode_MRI) {
offset = Operand(i.InputInt64(1));
} else {
DCHECK_EQ(addressing_mode, kMode_MRR);
offset = Operand(i.InputRegister(1));
}
Register value = i.InputRegister(2);
#if DEBUG
IndirectPointerTag tag = static_cast<IndirectPointerTag>(i.InputInt64(3));
DCHECK(IsValidIndirectPointerTag(tag));
#endif // DEBUG
RecordTrapInfoIfNeeded(zone(), this, opcode, instr, __ pc_offset());
__ StoreIndirectPointerField(value, MemOperand(object, offset));
break;
}Darius Mercadier . unresolved
Same here: you're not verifying that it's valid to skip the WB, so do you need this?
File src/compiler/backend/arm64/instruction-selector-arm64.cc
Line 1286, Patchset 25 (Parent):
code |= RecordWriteModeField::encode(record_write_mode);Darius Mercadier . unresolved
You could probably keep this here, since your NoWB stores ignore the RecordWriteModeField anyways.
File src/compiler/backend/instruction-codes.h
Line 112, Patchset 25 (Latest):
V(ArchStoreNoWriteBarrier) \
V(ArchAtomicStoreNoWriteBarrier) \
V(ArchStoreIndirectNoWriteBarrier) \Darius Mercadier . unresolved
I would name these `SkippedWB` rather than `NoWB`, to make it clearer that they aren't supposed to be called as soon as there is a store without WB, but rather as soon as there is an elided WB and the verify_write_barriers flag.
File src/compiler/backend/x64/code-generator-x64.cc
Line 1966, Patchset 25 (Latest):
case kArchStoreIndirectNoWriteBarrier: {
size_t index = 0;
Operand operand = i.MemoryOperand(&index);
Register value = i.InputRegister(index++);
#if DEBUG
IndirectPointerTag tag =
static_cast<IndirectPointerTag>(i.InputInt64(index));
DCHECK(IsValidIndirectPointerTag(tag));
#endif // DEBUG
EmitTSANAwareStore<std::memory_order_relaxed>(
zone(), this, masm(), operand, value, i, DetermineStubCallMode(),
MachineRepresentation::kIndirectPointer, instr);
break;
}Darius Mercadier . unresolved
You're not verifying anything here; is it by design?
File src/compiler/turboshaft/memory-optimization-reducer.h
Line 220, Patchset 22:
const WriteBarrierKind write_barrier_kind =
v8_flags.verify_write_barriers
? WriteBarrierKind::kSkippedWriteBarrier
: WriteBarrierKind::kNoWriteBarrier;Darius Mercadier . unresolved
Maybe make this a class variable to avoid re-checking v8_flags.verify_write_barriers for every elided WB.
File src/compiler/write-barrier-kind.h
Line 20, Patchset 25 (Latest):
kSkippedWriteBarrier,Darius Mercadier . unresolved
Maybe add a comment at the end of the line explaining what this is.
Related details
Attention is currently required from:
- Dominik Inführ
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Sep 4, 2025, 8:31:42 AM (3 days ago) Sep 4
to Darius Mercadier, AyeAye, V8 LUCI CQ, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Darius Mercadier
Dominik Inführ added 6 comments![]( "Open in Gerrit")
File src/compiler/backend/arm64/code-generator-arm64.cc
Line 1399, Patchset 25:
case kArchStoreIndirectNoWriteBarrier: {
AddressingMode addressing_mode =
AddressingModeField::decode(instr->opcode());
Register object = i.InputRegister(0);
Operand offset(0);
if (addressing_mode == kMode_MRI) {
offset = Operand(i.InputInt64(1));
} else {
DCHECK_EQ(addressing_mode, kMode_MRR);
offset = Operand(i.InputRegister(1));
}
Register value = i.InputRegister(2);
#if DEBUG
IndirectPointerTag tag = static_cast<IndirectPointerTag>(i.InputInt64(3));
DCHECK(IsValidIndirectPointerTag(tag));
#endif // DEBUG
RecordTrapInfoIfNeeded(zone(), this, opcode, instr, __ pc_offset());
__ StoreIndirectPointerField(value, MemOperand(object, offset));
break;
}Darius Mercadier . resolved
Same here: you're not verifying that it's valid to skip the WB, so do you need this?
Dominik Inführ
As discussed offline, I will add verification for those stores as a follow-up CL.
File src/compiler/backend/arm64/instruction-selector-arm64.cc
Line 1286, Patchset 25 (Parent):
code |= RecordWriteModeField::encode(record_write_mode);Darius Mercadier . unresolved
You could probably keep this here, since your NoWB stores ignore the RecordWriteModeField anyways.
Dominik Inführ
I changed this because `WriteBarrierKindToRecordWriteMode` does not expect `kSkippedWriteBarrier`. So I would need to at least assign to `record_write_mode` in the branches. And I would need to assign some value for kSkippedWriteBarrier as well.
Alternatively I could make `WriteBarrierKindToRecordWriteMode` aware of kSkippedWriteBarrier - but I wasn't too happy about this. Wdyt?
File src/compiler/backend/instruction-codes.h
Line 112, Patchset 25:
V(ArchStoreNoWriteBarrier) \
V(ArchAtomicStoreNoWriteBarrier) \
V(ArchStoreIndirectNoWriteBarrier) \Darius Mercadier . resolved
I would name these `SkippedWB` rather than `NoWB`, to make it clearer that they aren't supposed to be called as soon as there is a store without WB, but rather as soon as there is an elided WB and the verify_write_barriers flag.
Dominik Inführ
I like it, thanks!
File src/compiler/backend/x64/code-generator-x64.cc
Line 1966, Patchset 25:
case kArchStoreIndirectNoWriteBarrier: {
size_t index = 0;
Operand operand = i.MemoryOperand(&index);
Register value = i.InputRegister(index++);
#if DEBUG
IndirectPointerTag tag =
static_cast<IndirectPointerTag>(i.InputInt64(index));
DCHECK(IsValidIndirectPointerTag(tag));
#endif // DEBUG
EmitTSANAwareStore<std::memory_order_relaxed>(
zone(), this, masm(), operand, value, i, DetermineStubCallMode(),
MachineRepresentation::kIndirectPointer, instr);
break;
}Darius Mercadier . resolved
You're not verifying anything here; is it by design?
Dominik Inführ
As above.
File src/compiler/turboshaft/memory-optimization-reducer.h
Line 220, Patchset 22:
const WriteBarrierKind write_barrier_kind =
v8_flags.verify_write_barriers
? WriteBarrierKind::kSkippedWriteBarrier
: WriteBarrierKind::kNoWriteBarrier;Darius Mercadier . resolved
Maybe make this a class variable to avoid re-checking v8_flags.verify_write_barriers for every elided WB.
Dominik Inführ
Done
File src/compiler/write-barrier-kind.h
Maybe add a comment at the end of the line explaining what this is.
Attention is currently required from:
- Darius Mercadier
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Sep 4, 2025, 8:32:28 AM (3 days ago) Sep 4
to Darius Mercadier, AyeAye, V8 LUCI CQ, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Darius Mercadier
Dominik Inführ added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 29 (Latest):
Dominik Inführ . resolved
I believe I have addressed your feedback. PTALA
Related details
Attention is currently required from:
- Darius Mercadier
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Sep 4, 2025, 8:36:42 AM (3 days ago) Sep 4
to Dominik Inführ, AyeAye, V8 LUCI CQ, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ
Darius Mercadier voted and added 2 comments![]( "Open in Gerrit")
Votes added by Darius Mercadier
| Code-Review | +1 |
2 comments
Patchset-level comments
Darius Mercadier . resolved
Still LGTM, thanks :)
File src/compiler/backend/arm64/instruction-selector-arm64.cc
Line 1286, Patchset 25 (Parent):
code |= RecordWriteModeField::encode(record_write_mode);Darius Mercadier . resolved
Dominik InführYou could probably keep this here, since your NoWB stores ignore the RecordWriteModeField anyways.
I changed this because `WriteBarrierKindToRecordWriteMode` does not expect `kSkippedWriteBarrier`. So I would need to at least assign to `record_write_mode` in the branches. And I would need to assign some value for kSkippedWriteBarrier as well.
Alternatively I could make `WriteBarrierKindToRecordWriteMode` aware of kSkippedWriteBarrier - but I wasn't too happy about this. Wdyt?
Darius Mercadier
Ah right I see. Your current version looks good then :)
Related details
Attention is currently required from:
- Dominik Inführ
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Sep 4, 2025, 9:22:36 AM (3 days ago) Sep 4
to Darius Mercadier, AyeAye, V8 LUCI CQ, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Sep 4, 2025, 9:24:43 AM (3 days ago) Sep 4
to Dominik Inführ, Darius Mercadier, AyeAye, Hannes Payer, v8-mip...@googlegroups.com, v8-risc...@chromium.org, v8-ppc...@googlegroups.com, dmercadi...@chromium.org, mlippau...@chromium.org, v8-flag...@chromium.org, v8-re...@googlegroups.com
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[compiler] Support skipped write barrier verification in Turbofan
This CL adds skipped write barrier verification support to
Turbofan. For this the compiler generates calls to
Heap::VerifySkippedWriteBarrier to all stores which got their
write barrier eliminated during compilation.
For this we need to add a new WriteBarrierKind kSkippedWriteBarrier
which is used by the compiler instead of kNoWriteBarrier when
eliminating write barriers. We need this additional state to different
stores with skipped write barriers from stores that cannot have a
write barrier (e.g. Float64).
Note that we only use kSkippedWriteBarrier when write barrier
verification is enabled, production code should not be affected.
Bug: 437096305
Change-Id: I303155ca769af209b443af0a9f547f99e8997ca0
Cq-Include-Trybots: luci.v8.try:v8_mac_arm64_dbg
Cq-Include-Trybots: luci.v8.try:v8_linux_arm_dbg
Cq-Include-Trybots: luci.v8.try:v8_linux_dbg
Reviewed-by: Darius Mercadier <dmerc...@chromium.org>
Commit-Queue: Dominik Inführ <dinf...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#102246}
Files:
- M src/codegen/arm/macro-assembler-arm.cc
- M src/codegen/arm64/macro-assembler-arm64.cc
- M src/codegen/ia32/macro-assembler-ia32.cc
- M src/compiler/backend/arm/code-generator-arm.cc
- M src/compiler/backend/arm/instruction-selector-arm.cc
- M src/compiler/backend/arm64/code-generator-arm64.cc
- M src/compiler/backend/arm64/instruction-selector-arm64.cc
- M src/compiler/backend/ia32/code-generator-ia32.cc
- M src/compiler/backend/ia32/instruction-selector-ia32.cc
- M src/compiler/backend/instruction-codes.h
- M src/compiler/backend/instruction-scheduler.cc
- M src/compiler/backend/x64/code-generator-x64.cc
- M src/compiler/backend/x64/instruction-selector-x64.cc
- M src/compiler/machine-operator.cc
- M src/compiler/turboshaft/memory-optimization-reducer.h
- M src/compiler/turboshaft/stack-check-lowering-reducer.h
- M src/compiler/write-barrier-kind.h
- M src/heap/heap.cc
Change size: L
Delta: 18 files changed, 422 insertions(+), 34 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Darius Mercadier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kasper Lund (Gerrit)
Sep 5, 2025, 6:32:06 AM (2 days ago) Sep 5
to Florian Loitsch, Yahan Lu (LuYahan), V8 LUCI CQ, AyeAye, v8-re...@googlegroups.com, dmercadi...@chromium.org
Attention needed from Florian Loitsch and Yahan Lu (LuYahan)
Kasper Lund added 3 comments![]( "Open in Gerrit")
File src/codegen/riscv/macro-assembler-riscv.cc
Line 4672, Patchset 3 (Parent):
BlockTrampolinePoolScope block_trampoline_pool(this);Kasper Lund . resolved
We were already in a block scope here, set up at the beginning of this method (`BranchShortHelper`).
Line 4687, Patchset 3 (Latest):
// TODO(kas...@rivosinc.com): This probably has no effect, because theKasper Lund . resolved
This comment is the only change for the switch except for the indentation change.
Line 5442, Patchset 3 (Latest):
EmitConstPoolWithoutJumpIfNeeded();Kasper Lund . resolved
I'll do another pass where we clean up the constant pool handling. It is really hard to make sure we do it in all the right places. For now, I just want to unblock fixing the debug builds.
Related details
Attention is currently required from:
- Florian Loitsch
- Yahan Lu (LuYahan)
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Loitsch (Gerrit)
Sep 5, 2025, 7:32:46 AM (2 days ago) Sep 5
to Kasper Lund, Yahan Lu (LuYahan), V8 LUCI CQ, AyeAye, v8-re...@googlegroups.com, dmercadi...@chromium.org
Attention needed from Kasper Lund and Yahan Lu (LuYahan)
Florian Loitsch voted and added 1 comment![]( "Open in Gerrit")
Votes added by Florian Loitsch
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Kasper Lund
- Yahan Lu (LuYahan)
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kasper Lund (Gerrit)
Sep 5, 2025, 11:13:53 AM (2 days ago) Sep 5
to Florian Loitsch, Yahan Lu (LuYahan), V8 LUCI CQ, AyeAye, v8-re...@googlegroups.com, dmercadi...@chromium.org
Attention needed from Yahan Lu (LuYahan)
Kasper Lund voted Commit-Queue+2![]( "Open in Gerrit")
Attention is currently required from:
- Yahan Lu (LuYahan)
Submit Requirements:
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Sep 5, 2025, 11:15:40 AM (2 days ago) Sep 5
to Kasper Lund, Florian Loitsch, Yahan Lu (LuYahan), AyeAye, v8-re...@googlegroups.com, dmercadi...@chromium.org
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[riscv][compiler] Support skipped write barrier verification in Turbofan
Port commit 23635da6abc21671350eaed0a34ce75fb9ffe8fe.
Bug: 437096305
Change-Id: Ied3def141c3fcf3e8c53bc866639089e1e9b6ae9
Reviewed-by: Florian Loitsch <floi...@rivosinc.com>
Commit-Queue: Kasper Lund <kas...@rivosinc.com>
Cr-Commit-Position: refs/heads/main@{#102285}
Files:
- M src/codegen/riscv/macro-assembler-riscv.cc
- M src/compiler/backend/riscv/code-generator-riscv.cc
- M src/compiler/backend/riscv/instruction-selector-riscv32.cc
- M src/compiler/backend/riscv/instruction-selector-riscv64.cc
Change size: L
Delta: 4 files changed, 286 insertions(+), 152 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Florian Loitsch
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages