[instruction-selector-x64] Add missing CanCover check

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. In this case the
effect level of A and B might differ.

This CL adds a missing CanCover check to a case of shift reduction where
we assumed transitivity.

Change-Id: I9f368ffa6907d2af21bbc87b3e6570d0d422e125
Bug: v8:8384
Reviewed-on: https://chromium-review.googlesource.com/c/1307419
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57157}

src/compiler/instruction-selector.cc

@@ -282,6 +282,21 @@ 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);

src/compiler/instruction-selector.h

@@ -393,6 +393,10 @@ class V8_EXPORT_PRIVATE InstructionSelector final {
   // 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.
   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,

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

@@ -766,6 +766,8 @@ bool TryMatchLoadWord64AndShiftRight(InstructionSelector* selector, Node* node,
   Int64BinopMatcher m(node);
   if (selector->CanCover(m.node(), m.left().node()) && m.left().IsLoad() &&
       m.right().Is(32)) {
+    DCHECK_EQ(selector->GetEffectLevel(node),
+              selector->GetEffectLevel(m.left().node()));
     // Just load and sign-extend the interesting 4 bytes instead. This happens,
     // for example, when we're loading and untagging SMIs.
     BaseWithIndexAndDisplacement64Matcher mleft(m.left().node(),
@@ -1393,7 +1395,8 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) {
       case IrOpcode::kWord64Shr: {
         Int64BinopMatcher m(value);
         if (m.right().Is(32)) {
-          if (TryMatchLoadWord64AndShiftRight(this, value, kX64Movl)) {
+          if (CanCoverTransitively(node, value, value->InputAt(0)) &&
+              TryMatchLoadWord64AndShiftRight(this, value, kX64Movl)) {
             return EmitIdentity(node);
           }
           Emit(kX64Shr, g.DefineSameAsFirst(node),

test/mjsunit/regress/regress-8384.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+function assert(cond) { if (!cond) throw "Assert"; }
+
+function Constructor() {
+  this.padding1 = null;
+  this.padding2 = null;
+  this.padding3 = null;
+  this.padding4 = null;
+  this.padding5 = null;
+  this.padding6 = null;
+  this.padding7 = null;
+  this.padding8 = null;
+  this.padding9 = null;
+  this.padding10 = null;
+  this.padding11 = null;
+  this.padding12 = null;
+  this.padding13 = null;
+  this.padding14 = null;
+  this.padding15 = null;
+  this.padding16 = null;
+  this.padding17 = null;
+  this.padding18 = null;
+  this.padding19 = null;
+  this.padding20 = null;
+  this.padding21 = null;
+  this.padding22 = null;
+  this.padding23 = null;
+  this.padding24 = null;
+  this.padding25 = null;
+  this.padding26 = null;
+  this.padding27 = null;
+  this.padding28 = null;
+  this.padding29 = null;
+  this.array = null;
+  this.accumulator = 0;
+}
+
+function f(k) {
+  var c = k.accumulator | 0;
+  k.accumulator = k.array[(k.accumulator + 1 | 0)] | 0;
+  k.array[c + 1 | 0] = (-1);
+  var head = k.accumulator;
+  assert((head + c) & 1);
+  while (head >= 0) {
+    head = k.array[head + 1 | 0];
+  }
+  return;
+}
+
+const tmp = new Constructor();
+tmp.array = new Int32Array(5);
+for (var i = 1; i < 5; i++)
+  tmp.array[i] = i | 0;
+tmp.accumulator = 0;
+
+f(tmp);
+f(tmp);
+%OptimizeFunctionOnNextCall(f);
+f(tmp);  // This must not trigger the {assert}.