Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick 13ffdf63a471 from v8 #34881

Merged
merged 2 commits into from Jul 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/v8/.patches
Expand Up @@ -10,3 +10,4 @@ revert_fix_cppgc_removed_deleted_cstors_in_cppheapcreateparams.patch
cherry-pick-723ed8a9cfff.patch
cherry-pick-44c4e56fea2c.patch
version_10_2_154_10_cherry-pick.patch
cherry-pick-13ffdf63a471.patch
169 changes: 169 additions & 0 deletions patches/v8/cherry-pick-13ffdf63a471.patch
@@ -0,0 +1,169 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Tobias Tebbi <tebbi@chromium.org>
Date: Fri, 17 Jun 2022 10:14:44 +0000
Subject: Merged: [compiler] make CanCover() transitive

In addition to checking that a node is owned, CanCover() also needs to
check if there are any side-effects in between the current node and
the merged node. When merging inputs of inputs, this check was done
with the wrong side-effect level of the in-between node.
We partially fixed this before with `CanCoverTransitively`.
This CL addresses the issue by always comparing to the side-effect
level of the node from which we started, making `CanCoverTransitively`
superfluous.

Bug: chromium:1336869
(cherry picked from commit 6048f754931e0971cab58fb0de785482d175175b)

Change-Id: I63cf6bfdd40c2c55921db4a2ac737612bb7da4e4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3722095
Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Cr-Commit-Position: refs/branch-heads/10.2@{#19}
Cr-Branched-From: 374091f382e88095694c1283cbdc2acddc1b1417-refs/heads/10.2.154@{#1}
Cr-Branched-From: f0c353f6315eeb2212ba52478983a3b3af07b5b1-refs/heads/main@{#79976}

diff --git a/src/compiler/backend/instruction-selector.cc b/src/compiler/backend/instruction-selector.cc
index 528aa543688041b816087efa02dec81ad8c9e3b1..b0e589738a0a1d7fb9d7f8eeb7677a5d7d0df022 100644
--- a/src/compiler/backend/instruction-selector.cc
+++ b/src/compiler/backend/instruction-selector.cc
@@ -283,7 +283,7 @@ Instruction* InstructionSelector::Emit(Instruction* instr) {

bool InstructionSelector::CanCover(Node* user, Node* node) const {
// 1. Both {user} and {node} must be in the same basic block.
- if (schedule()->block(node) != schedule()->block(user)) {
+ if (schedule()->block(node) != current_block_) {
return false;
}
// 2. Pure {node}s must be owned by the {user}.
@@ -291,7 +291,7 @@ bool InstructionSelector::CanCover(Node* user, Node* node) const {
return node->OwnedBy(user);
}
// 3. Impure {node}s must match the effect level of {user}.
- if (GetEffectLevel(node) != GetEffectLevel(user)) {
+ if (GetEffectLevel(node) != current_effect_level_) {
return false;
}
// 4. Only {node} must have value edges pointing to {user}.
@@ -303,21 +303,6 @@ bool InstructionSelector::CanCover(Node* user, Node* node) const {
return true;
}

-bool InstructionSelector::CanCoverTransitively(Node* user, Node* node,
- Node* node_input) const {
- if (CanCover(user, node) && CanCover(node, node_input)) {
- // If {node} is pure, transitivity might not hold.
- if (node->op()->HasProperty(Operator::kPure)) {
- // If {node_input} is pure, the effect levels do not matter.
- if (node_input->op()->HasProperty(Operator::kPure)) return true;
- // Otherwise, {user} and {node_input} must have the same effect level.
- return GetEffectLevel(user) == GetEffectLevel(node_input);
- }
- return true;
- }
- return false;
-}
-
bool InstructionSelector::IsOnlyUserOfNodeInSameBlock(Node* user,
Node* node) const {
BasicBlock* bb_user = schedule()->block(user);
@@ -1206,6 +1191,7 @@ void InstructionSelector::VisitBlock(BasicBlock* block) {
int effect_level = 0;
for (Node* const node : *block) {
SetEffectLevel(node, effect_level);
+ current_effect_level_ = effect_level;
if (node->opcode() == IrOpcode::kStore ||
node->opcode() == IrOpcode::kUnalignedStore ||
node->opcode() == IrOpcode::kCall ||
@@ -1223,6 +1209,7 @@ void InstructionSelector::VisitBlock(BasicBlock* block) {
// control input should be on the same effect level as the last node.
if (block->control_input() != nullptr) {
SetEffectLevel(block->control_input(), effect_level);
+ current_effect_level_ = effect_level;
}

auto FinishEmittedInstructions = [&](Node* node, int instruction_start) {
diff --git a/src/compiler/backend/instruction-selector.h b/src/compiler/backend/instruction-selector.h
index b33de8e8569ebce59eebfec6eef12ed1f1874ddc..167cdc1f0c4962d55f3f9b40b7865737fc652bb0 100644
--- a/src/compiler/backend/instruction-selector.h
+++ b/src/compiler/backend/instruction-selector.h
@@ -423,15 +423,12 @@ class V8_EXPORT_PRIVATE InstructionSelector final {
// Used in pattern matching during code generation.
// Check if {node} can be covered while generating code for the current
// instruction. A node can be covered if the {user} of the node has the only
- // edge and the two are in the same basic block.
- // Before fusing two instructions a and b, it is useful to check that
- // CanCover(a, b) holds. If this is not the case, code for b must still be
- // generated for other users, and fusing is unlikely to improve performance.
+ // edge, the two are in the same basic block, and there are no side-effects
+ // in-between. The last check is crucial for soundness.
+ // For pure nodes, CanCover(a,b) is checked to avoid duplicated execution:
+ // If this is not the case, code for b must still be generated for other
+ // users, and fusing is unlikely to improve performance.
bool CanCover(Node* user, Node* node) const;
- // CanCover is not transitive. The counter example are Nodes A,B,C such that
- // CanCover(A, B) and CanCover(B,C) and B is pure: The the effect level of A
- // and B might differ. CanCoverTransitively does the additional checks.
- bool CanCoverTransitively(Node* user, Node* node, Node* node_input) const;

// Used in pattern matching during code generation.
// This function checks that {node} and {user} are in the same basic block,
@@ -756,6 +753,7 @@ class V8_EXPORT_PRIVATE InstructionSelector final {
BoolVector defined_;
BoolVector used_;
IntVector effect_level_;
+ int current_effect_level_;
IntVector virtual_registers_;
IntVector virtual_register_rename_;
InstructionScheduler* scheduler_;
diff --git a/src/compiler/backend/loong64/instruction-selector-loong64.cc b/src/compiler/backend/loong64/instruction-selector-loong64.cc
index 4f03f99acd5b859b6fc095a34dea66ec058bcd4b..6b2d25f5dc1d912c1183e1b349bd29615024fa48 100644
--- a/src/compiler/backend/loong64/instruction-selector-loong64.cc
+++ b/src/compiler/backend/loong64/instruction-selector-loong64.cc
@@ -1446,7 +1446,7 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) {
if (CanCover(node, value)) {
switch (value->opcode()) {
case IrOpcode::kWord64Sar: {
- if (CanCoverTransitively(node, value, value->InputAt(0)) &&
+ if (CanCover(value, value->InputAt(0)) &&
TryEmitExtendingLoad(this, value, node)) {
return;
} else {
diff --git a/src/compiler/backend/mips64/instruction-selector-mips64.cc b/src/compiler/backend/mips64/instruction-selector-mips64.cc
index 4f5738ddaddba03aaa0ebbc351fe18d766e04d00..1e29e0ed730be49f4a09484d6c4db3ffca92355e 100644
--- a/src/compiler/backend/mips64/instruction-selector-mips64.cc
+++ b/src/compiler/backend/mips64/instruction-selector-mips64.cc
@@ -1532,7 +1532,7 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) {
if (CanCover(node, value)) {
switch (value->opcode()) {
case IrOpcode::kWord64Sar: {
- if (CanCoverTransitively(node, value, value->InputAt(0)) &&
+ if (CanCover(value, value->InputAt(0)) &&
TryEmitExtendingLoad(this, value, node)) {
return;
} else {
diff --git a/src/compiler/backend/riscv64/instruction-selector-riscv64.cc b/src/compiler/backend/riscv64/instruction-selector-riscv64.cc
index b6ced36f796b922df19e8bbb70db1bb6c33a2149..3b744795e5cd83ba4a8eff23cf4e607ffa0ca1c5 100644
--- a/src/compiler/backend/riscv64/instruction-selector-riscv64.cc
+++ b/src/compiler/backend/riscv64/instruction-selector-riscv64.cc
@@ -1428,7 +1428,7 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) {
if (CanCover(node, value)) {
switch (value->opcode()) {
case IrOpcode::kWord64Sar: {
- if (CanCoverTransitively(node, value, value->InputAt(0)) &&
+ if (CanCover(value, value->InputAt(0)) &&
TryEmitExtendingLoad(this, value, node)) {
return;
} else {
diff --git a/src/compiler/backend/x64/instruction-selector-x64.cc b/src/compiler/backend/x64/instruction-selector-x64.cc
index 3aec65221fb03e0d4143d08f5156b8a3f192e82c..5b8064010419d2749cddc0302ba63dc20f6e58df 100644
--- a/src/compiler/backend/x64/instruction-selector-x64.cc
+++ b/src/compiler/backend/x64/instruction-selector-x64.cc
@@ -1802,7 +1802,7 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) {
case IrOpcode::kWord64Shr: {
Int64BinopMatcher m(value);
if (m.right().Is(32)) {
- if (CanCoverTransitively(node, value, value->InputAt(0)) &&
+ if (CanCover(value, value->InputAt(0)) &&
TryMatchLoadWord64AndShiftRight(this, value, kX64Movl)) {
return EmitIdentity(node);
}