[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
Change-Id: I78479b32461ede81138f8b5d48d60058cfb5fa0a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3707277Reviewed-by: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81217}

src/compiler/backend/instruction-selector.cc

@@ -284,7 +284,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}.
@@ -292,7 +292,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}.
@@ -304,21 +304,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);
@@ -1196,6 +1181,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 ||
@@ -1213,6 +1199,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) {

src/compiler/backend/instruction-selector.h

@@ -407,15 +407,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,
@@ -749,6 +746,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_;

src/compiler/backend/loong64/instruction-selector-loong64.cc

@@ -1448,7 +1448,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 {

src/compiler/backend/mips64/instruction-selector-mips64.cc

@@ -1534,7 +1534,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 {

src/compiler/backend/riscv64/instruction-selector-riscv64.cc

@@ -1481,7 +1481,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 {

src/compiler/backend/x64/instruction-selector-x64.cc

@@ -1824,7 +1824,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);
           }