[turbofan] Disallow floating control in wasm

Loop unrolling did not work properly with floating control. Seeing as
very few spots in the wasm compiler introduced floating control, we
decided to disallow it altogether.
Changes:
- When lowering 64-bit rol/ror/clz/ctz in 32-bit platforms, we use a
  diamond operator, which used to introduce floating control. This CL
  adds a control edge to these operators so that the diamond can be
  chained to that control instead.
- During loop analysis, as an additional safety check, we check that the
  explored loop does not have floating control. Exceptionally, floating
  control pointing directly do start() is allowed.
- Change wasm-compiler so that generated floating projections point to
  start() even after stack check patch-in.

Bug: chromium:1184929, v8:11298
Change-Id: I1ee063f5250037ae6c84d2f16b0bd8fff3923117
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2876851Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74527}

src/compiler/code-assembler.h

@@ -310,7 +310,6 @@ class CodeAssemblerParameterizedLabel;
   V(Word64And, Word64T, Word64T, Word64T)                               \
   V(Word64Or, Word64T, Word64T, Word64T)                                \
   V(Word64Xor, Word64T, Word64T, Word64T)                               \
-  V(Word64Ror, Word64T, Word64T, Word64T)                               \
   V(Word64Shl, Word64T, Word64T, Word64T)                               \
   V(Word64Shr, Word64T, Word64T, Word64T)                               \
   V(Word64Sar, Word64T, Word64T, Word64T)

src/compiler/graph.h

@@ -67,7 +67,7 @@ class V8_EXPORT_PRIVATE Graph final : public NON_EXPORTED_BASE(ZoneObject) {
 
   // Factory template for nodes with static input counts.
   // Note: Template magic below is used to ensure this method is only considered
-  // for argument types convertible to Node* during overload resoluation.
+  // for argument types convertible to Node* during overload resolution.
   template <typename... Nodes,
             typename = typename std::enable_if_t<
                 base::all(std::is_convertible<Nodes, Node*>::value...)>>

src/compiler/int64-lowering.cc

@@ -392,8 +392,7 @@ void Int64Lowering::LowerNode(Node* node) {
             if (call_descriptor->GetReturnType(old_index).representation() ==
                 MachineRepresentation::kWord64) {
               Node* high_node = graph()->NewNode(
-                  common()->Projection(new_index + 1), node,
-                  graph()->start());
+                  common()->Projection(new_index + 1), node, graph()->start());
               ReplaceNode(use_node, use_node, high_node);
               ++new_index;
             }
@@ -684,11 +683,11 @@ void Int64Lowering::LowerNode(Node* node) {
       ReplaceNode(node, low_node, high_node);
       break;
     }
-    case IrOpcode::kWord64Rol:
+    case IrOpcode::kWord64RolLowerable:
       DCHECK(machine()->Word32Rol().IsSupported());
       V8_FALLTHROUGH;
-    case IrOpcode::kWord64Ror: {
-      DCHECK_EQ(2, node->InputCount());
+    case IrOpcode::kWord64RorLowerable: {
+      DCHECK_EQ(3, node->InputCount());
       Node* input = node->InputAt(0);
       Node* shift = HasReplacementLow(node->InputAt(1))
                         ? GetReplacementLow(node->InputAt(1))
@@ -721,7 +720,7 @@ void Int64Lowering::LowerNode(Node* node) {
 
           auto* op1 = machine()->Word32Shr();
           auto* op2 = machine()->Word32Shl();
-          bool is_ror = node->opcode() == IrOpcode::kWord64Ror;
+          bool is_ror = node->opcode() == IrOpcode::kWord64RorLowerable;
           if (!is_ror) std::swap(op1, op2);
 
           Node* low_node =
@@ -742,7 +741,7 @@ void Int64Lowering::LowerNode(Node* node) {
                                graph()->NewNode(common()->Int32Constant(0x1F)));
         }
 
-        bool is_ror = node->opcode() == IrOpcode::kWord64Ror;
+        bool is_ror = node->opcode() == IrOpcode::kWord64RorLowerable;
         Node* inv_mask =
             is_ror ? graph()->NewNode(
                          machine()->Word32Xor(),
@@ -774,6 +773,7 @@ void Int64Lowering::LowerNode(Node* node) {
             graph(), common(),
             graph()->NewNode(machine()->Int32LessThan(), masked_shift6,
                              graph()->NewNode(common()->Int32Constant(32))));
+        lt32.Chain(NodeProperties::GetControlInput(node));
 
         // The low word and the high word can be swapped either at the input or
         // at the output. We swap the inputs so that shift does not have to be
@@ -807,13 +807,14 @@ void Int64Lowering::LowerNode(Node* node) {
       }
       break;
     }
-    case IrOpcode::kWord64Clz: {
-      DCHECK_EQ(1, node->InputCount());
+    case IrOpcode::kWord64ClzLowerable: {
+      DCHECK_EQ(2, node->InputCount());
       Node* input = node->InputAt(0);
       Diamond d(
           graph(), common(),
           graph()->NewNode(machine()->Word32Equal(), GetReplacementHigh(input),
                            graph()->NewNode(common()->Int32Constant(0))));
+      d.Chain(NodeProperties::GetControlInput(node));
 
       Node* low_node = d.Phi(
           MachineRepresentation::kWord32,
@@ -825,14 +826,16 @@ void Int64Lowering::LowerNode(Node* node) {
       ReplaceNode(node, low_node, graph()->NewNode(common()->Int32Constant(0)));
       break;
     }
-    case IrOpcode::kWord64Ctz: {
-      DCHECK_EQ(1, node->InputCount());
+    case IrOpcode::kWord64CtzLowerable: {
+      DCHECK_EQ(2, node->InputCount());
       DCHECK(machine()->Word32Ctz().IsSupported());
       Node* input = node->InputAt(0);
       Diamond d(
           graph(), common(),
           graph()->NewNode(machine()->Word32Equal(), GetReplacementLow(input),
                            graph()->NewNode(common()->Int32Constant(0))));
+      d.Chain(NodeProperties::GetControlInput(node));
+
       Node* low_node =
           d.Phi(MachineRepresentation::kWord32,
                 graph()->NewNode(machine()->Int32Add(),
@@ -844,6 +847,12 @@ void Int64Lowering::LowerNode(Node* node) {
       ReplaceNode(node, low_node, graph()->NewNode(common()->Int32Constant(0)));
       break;
     }
+    case IrOpcode::kWord64Ror:
+    case IrOpcode::kWord64Rol:
+    case IrOpcode::kWord64Ctz:
+    case IrOpcode::kWord64Clz:
+      FATAL("%s operator should not be used in 32-bit systems",
+            node->op()->mnemonic());
     case IrOpcode::kWord64Popcnt: {
       DCHECK_EQ(1, node->InputCount());
       Node* input = node->InputAt(0);

src/compiler/loop-analysis.cc

@@ -547,7 +547,6 @@ LoopTree* LoopFinder::BuildLoopTree(Graph* graph, TickCounter* tick_counter,
 ZoneUnorderedSet<Node*>* LoopFinder::FindUnnestedLoopFromHeader(
     Node* loop_header, Zone* zone, size_t max_size) {
   auto* visited = zone->New<ZoneUnorderedSet<Node*>>(zone);
-
   std::vector<Node*> queue;
 
   DCHECK(loop_header->opcode() == IrOpcode::kLoop);
@@ -589,6 +588,30 @@ ZoneUnorderedSet<Node*>* LoopFinder::FindUnnestedLoopFromHeader(
     }
   }
 
+  // Check that there is no floating control other than direct nodes to start().
+  // We do this by checking that all non-start control inputs of loop nodes are
+  // also in the loop.
+  // TODO(manoskouk): This is a safety check. Consider making it DEBUG-only when
+  // we are confident there is no incompatible floating control generated in
+  // wasm.
+  for (Node* node : *visited) {
+    // The loop header is allowed to point outside the loop.
+    if (node == loop_header) continue;
+
+    for (Edge edge : node->input_edges()) {
+      Node* input = edge.to();
+      if (NodeProperties::IsControlEdge(edge) && visited->count(input) == 0 &&
+          input->opcode() != IrOpcode::kStart) {
+        FATAL(
+            "Floating control detected in wasm turbofan graph: Node #%d:%s is "
+            "inside loop headed by #%d, but its control dependency #%d:%s is "
+            "outside",
+            node->id(), node->op()->mnemonic(), loop_header->id(), input->id(),
+            input->op()->mnemonic());
+      }
+    }
+  }
+
   return visited;
 }
 

src/compiler/machine-operator.cc

@@ -252,6 +252,7 @@ std::ostream& operator<<(std::ostream& os, TruncateKind kind) {
   V(Word64Shl, Operator::kNoProperties, 2, 0, 1)                         \
   V(Word64Shr, Operator::kNoProperties, 2, 0, 1)                         \
   V(Word64Ror, Operator::kNoProperties, 2, 0, 1)                         \
+  V(Word64RorLowerable, Operator::kNoProperties, 2, 1, 1)                \
   V(Word64Equal, Operator::kCommutative, 2, 0, 1)                        \
   V(Int64Add, Operator::kAssociative | Operator::kCommutative, 2, 0, 1)  \
   V(Int64Sub, Operator::kNoProperties, 2, 0, 1)                          \
@@ -272,6 +273,7 @@ std::ostream& operator<<(std::ostream& os, TruncateKind kind) {
   PURE_BINARY_OP_LIST_64(V)                                                \
   V(Word32Clz, Operator::kNoProperties, 1, 0, 1)                           \
   V(Word64Clz, Operator::kNoProperties, 1, 0, 1)                           \
+  V(Word64ClzLowerable, Operator::kNoProperties, 1, 1, 1)                  \
   V(Word32ReverseBytes, Operator::kNoProperties, 1, 0, 1)                  \
   V(Word64ReverseBytes, Operator::kNoProperties, 1, 0, 1)                  \
   V(Simd128ReverseBytes, Operator::kNoProperties, 1, 0, 1)                 \
@@ -566,8 +568,10 @@ std::ostream& operator<<(std::ostream& os, TruncateKind kind) {
 #define PURE_OPTIONAL_OP_LIST(V)                            \
   V(Word32Ctz, Operator::kNoProperties, 1, 0, 1)            \
   V(Word64Ctz, Operator::kNoProperties, 1, 0, 1)            \
+  V(Word64CtzLowerable, Operator::kNoProperties, 1, 1, 1)   \
   V(Word32Rol, Operator::kNoProperties, 2, 0, 1)            \
   V(Word64Rol, Operator::kNoProperties, 2, 0, 1)            \
+  V(Word64RolLowerable, Operator::kNoProperties, 2, 1, 1)   \
   V(Word32ReverseBits, Operator::kNoProperties, 1, 0, 1)    \
   V(Word64ReverseBits, Operator::kNoProperties, 1, 0, 1)    \
   V(Int32AbsWithOverflow, Operator::kNoProperties, 1, 0, 2) \

src/compiler/machine-operator.h

@@ -249,26 +249,29 @@ class V8_EXPORT_PRIVATE MachineOperatorBuilder final
     kWord32ShiftIsSafe = 1u << 11,
     kWord32Ctz = 1u << 12,
     kWord64Ctz = 1u << 13,
-    kWord32Popcnt = 1u << 14,
-    kWord64Popcnt = 1u << 15,
-    kWord32ReverseBits = 1u << 16,
-    kWord64ReverseBits = 1u << 17,
-    kFloat32Select = 1u << 18,
-    kFloat64Select = 1u << 19,
-    kInt32AbsWithOverflow = 1u << 20,
-    kInt64AbsWithOverflow = 1u << 21,
-    kWord32Rol = 1u << 22,
-    kWord64Rol = 1u << 23,
-    kSatConversionIsSafe = 1u << 24,
-    kWord32Select = 1u << 25,
-    kWord64Select = 1u << 26,
+    kWord64CtzLowerable = 1u << 14,
+    kWord32Popcnt = 1u << 15,
+    kWord64Popcnt = 1u << 16,
+    kWord32ReverseBits = 1u << 17,
+    kWord64ReverseBits = 1u << 18,
+    kFloat32Select = 1u << 19,
+    kFloat64Select = 1u << 20,
+    kInt32AbsWithOverflow = 1u << 21,
+    kInt64AbsWithOverflow = 1u << 22,
+    kWord32Rol = 1u << 23,
+    kWord64Rol = 1u << 24,
+    kWord64RolLowerable = 1u << 25,
+    kSatConversionIsSafe = 1u << 26,
+    kWord32Select = 1u << 27,
+    kWord64Select = 1u << 28,
     kAllOptionalOps =
         kFloat32RoundDown | kFloat64RoundDown | kFloat32RoundUp |
         kFloat64RoundUp | kFloat32RoundTruncate | kFloat64RoundTruncate |
         kFloat64RoundTiesAway | kFloat32RoundTiesEven | kFloat64RoundTiesEven |
-        kWord32Ctz | kWord64Ctz | kWord32Popcnt | kWord64Popcnt |
-        kWord32ReverseBits | kWord64ReverseBits | kInt32AbsWithOverflow |
-        kInt64AbsWithOverflow | kWord32Rol | kWord64Rol | kSatConversionIsSafe |
+        kWord32Ctz | kWord64Ctz | kWord64CtzLowerable | kWord32Popcnt |
+        kWord64Popcnt | kWord32ReverseBits | kWord64ReverseBits |
+        kInt32AbsWithOverflow | kInt64AbsWithOverflow | kWord32Rol |
+        kWord64Rol | kWord64RolLowerable | kSatConversionIsSafe |
         kFloat32Select | kFloat64Select | kWord32Select | kWord64Select
   };
   using Flags = base::Flags<Flag, unsigned>;
@@ -389,10 +392,21 @@ class V8_EXPORT_PRIVATE MachineOperatorBuilder final
   const Operator* Word64SarShiftOutZeros() {
     return Word64Sar(ShiftKind::kShiftOutZeros);
   }
+
+  // 64-bit rol, ror, clz and ctz operators have two versions: the non-suffixed
+  // ones are meant to be used in 64-bit systems and have no control input. The
+  // "Lowerable"-suffixed ones are meant to be temporary operators in 32-bit
+  // systems and will be lowered to 32-bit operators. They have a control input
+  // to enable the lowering.
   const OptionalOperator Word64Rol();
   const Operator* Word64Ror();
   const Operator* Word64Clz();
   const OptionalOperator Word64Ctz();
+  const OptionalOperator Word64RolLowerable();
+  const Operator* Word64RorLowerable();
+  const Operator* Word64ClzLowerable();
+  const OptionalOperator Word64CtzLowerable();
+
   const Operator* Word64Equal();
 
   const Operator* Int32PairAdd();

src/compiler/opcodes.h

@@ -572,6 +572,8 @@
   V(Word64Sar)                   \
   V(Word64Rol)                   \
   V(Word64Ror)                   \
+  V(Word64RolLowerable)          \
+  V(Word64RorLowerable)          \
   V(Int64Add)                    \
   V(Int64AddWithOverflow)        \
   V(Int64Sub)                    \
@@ -690,6 +692,8 @@
   V(Word64Popcnt)                        \
   V(Word64Clz)                           \
   V(Word64Ctz)                           \
+  V(Word64ClzLowerable)                  \
+  V(Word64CtzLowerable)                  \
   V(Word64ReverseBits)                   \
   V(Word64ReverseBytes)                  \
   V(Simd128ReverseBytes)                 \

src/compiler/verifier.cc

@@ -482,8 +482,8 @@ void Verifier::Visitor::Check(Node* node, const AllNodes& all) {
       Node* control = NodeProperties::GetControlInput(node, 0);
       CHECK_EQ(effect_count, control->op()->ControlInputCount());
       CHECK_EQ(input_count, 1 + effect_count);
-      // If the control input is a Merge, then make sure that at least one
-      // of it's usages is non-phi.
+      // If the control input is a Merge, then make sure that at least one of
+      // its usages is non-phi.
       if (control->opcode() == IrOpcode::kMerge) {
         bool non_phi_use_found = false;
         for (Node* use : control->uses()) {
@@ -1667,8 +1667,12 @@ void Verifier::Visitor::Check(Node* node, const AllNodes& all) {
     case IrOpcode::kWord64Rol:
     case IrOpcode::kWord64Ror:
     case IrOpcode::kWord64Clz:
-    case IrOpcode::kWord64Popcnt:
     case IrOpcode::kWord64Ctz:
+    case IrOpcode::kWord64RolLowerable:
+    case IrOpcode::kWord64RorLowerable:
+    case IrOpcode::kWord64ClzLowerable:
+    case IrOpcode::kWord64CtzLowerable:
+    case IrOpcode::kWord64Popcnt:
     case IrOpcode::kWord64ReverseBits:
     case IrOpcode::kWord64ReverseBytes:
     case IrOpcode::kSimd128ReverseBytes:

src/compiler/wasm-compiler.cc

@@ -701,8 +701,23 @@ void WasmGraphBuilder::PatchInStackCheckIfNeeded() {
   if (effect() == dummy) return;
 
   // Now patch all control uses of {start} to use {control} and all effect uses
-  // to use {effect} instead. Then rewire the dummy node to use start instead.
+  // to use {effect} instead. We exclude Projection nodes: Projections pointing
+  // to start are floating control, and we want it to point directly to start
+  // because of restrictions later in the pipeline (specifically, loop
+  // unrolling).
+  // Then rewire the dummy node to use start instead.
   NodeProperties::ReplaceUses(start, start, effect(), control());
+  {
+    // We need an intermediate vector because we are not allowed to modify a use
+    // while traversing uses().
+    std::vector<Node*> projections;
+    for (Node* use : control()->uses()) {
+      if (use->opcode() == IrOpcode::kProjection) projections.emplace_back(use);
+    }
+    for (Node* use : projections) {
+      use->ReplaceInput(NodeProperties::FirstControlIndex(use), start);
+    }
+  }
   NodeProperties::ReplaceUses(dummy, nullptr, start, start);
 }
 
@@ -865,17 +880,19 @@ Node* WasmGraphBuilder::Binop(wasm::WasmOpcode opcode, Node* left, Node* right,
       std::swap(left, right);
       break;
     case wasm::kExprI64Ror:
-      op = m->Word64Ror();
       right = MaskShiftCount64(right);
-      break;
+      return m->Is64() ? graph()->NewNode(m->Word64Ror(), left, right)
+                       : graph()->NewNode(m->Word64RorLowerable(), left, right,
+                                          control());
     case wasm::kExprI64Rol:
       if (m->Word64Rol().IsSupported()) {
-        op = m->Word64Rol().op();
-        right = MaskShiftCount64(right);
-        break;
+        return m->Is64() ? graph()->NewNode(m->Word64Rol().op(), left,
+                                            MaskShiftCount64(right))
+                         : graph()->NewNode(m->Word64RolLowerable().op(), left,
+                                            MaskShiftCount64(right), control());
       } else if (m->Word32Rol().IsSupported()) {
-        op = m->Word64Rol().placeholder();
-        break;
+        return graph()->NewNode(m->Word64RolLowerable().placeholder(), left,
+                                right, control());
       }
       return BuildI64Rol(left, right);
     case wasm::kExprF32CopySign:
@@ -1166,19 +1183,22 @@ Node* WasmGraphBuilder::Unop(wasm::WasmOpcode opcode, Node* input,
       op = m->BitcastFloat64ToInt64();
       break;
     case wasm::kExprI64Clz:
-      op = m->Word64Clz();
-      break;
+      return m->Is64()
+                 ? graph()->NewNode(m->Word64Clz(), input)
+                 : graph()->NewNode(m->Word64ClzLowerable(), input, control());
     case wasm::kExprI64Ctz: {
-      OptionalOperator ctz64 = m->Word64Ctz();
-      if (ctz64.IsSupported()) {
-        op = ctz64.op();
-        break;
+      if (m->Word64Ctz().IsSupported()) {
+        return m->Is64() ? graph()->NewNode(m->Word64Ctz().op(), input)
+                         : graph()->NewNode(m->Word64CtzLowerable().op(), input,
+                                            control());
       } else if (m->Is32() && m->Word32Ctz().IsSupported()) {
-        op = ctz64.placeholder();
-        break;
+        return graph()->NewNode(m->Word64CtzLowerable().placeholder(), input,
+                                control());
       } else if (m->Word64ReverseBits().IsSupported()) {
         Node* reversed = graph()->NewNode(m->Word64ReverseBits().op(), input);
-        Node* result = graph()->NewNode(m->Word64Clz(), reversed);
+        Node* result = m->Is64() ? graph()->NewNode(m->Word64Clz(), reversed)
+                                 : graph()->NewNode(m->Word64ClzLowerable(),
+                                                    reversed, control());
         return result;
       } else {
         return BuildI64Ctz(input);

test/mjsunit/wasm/loop-unrolling.js

@@ -10,6 +10,29 @@
 load("test/mjsunit/wasm/wasm-module-builder.js");
 load("test/mjsunit/wasm/exceptions-utils.js");
 
+// Test that lowering a ror operator with int64-lowering does not produce
+// floating control, which is incompatible with loop unrolling.
+(function I64RorLoweringTest() {
+  let builder = new WasmModuleBuilder();
+  builder.addMemory(1000, 1000);
+
+  builder.addFunction("main", makeSig([kWasmI32, kWasmI64], []))
+    .addBody([
+      kExprLoop, kWasmVoid,
+        kExprLocalGet, 0x00,
+        kExprI32LoadMem, 0x00, 0x00,
+        kExprI64UConvertI32,
+        kExprLocalGet, 0x01,
+        kExprI64Ror,
+        kExprI32ConvertI64,
+        kExprBrIf, 0x00,
+      kExprEnd])
+    .exportFunc();
+
+  let module = new WebAssembly.Module(builder.toBuffer());
+  let instance = new WebAssembly.Instance(module);
+})();
+
 // Test the interaction between multireturn and loop unrolling.
 (function MultiBlockResultTest() {
   let builder = new WasmModuleBuilder();

test/unittests/compiler/machine-operator-unittest.cc

@@ -191,6 +191,7 @@ const PureOperator kPureOperators[] = {
     PURE(Word64Shr, 2, 0, 1),                 // --
     PURE(Word64Sar, 2, 0, 1),                 // --
     PURE(Word64Ror, 2, 0, 1),                 // --
+    PURE(Word64RorLowerable, 2, 1, 1),        // --
     PURE(Word64Equal, 2, 0, 1),               // --
     PURE(Int32Add, 2, 0, 1),                  // --
     PURE(Int32Sub, 2, 0, 1),                  // --
@@ -253,7 +254,6 @@ const PureOperator kPureOperators[] = {
 #undef PURE
 };
 
-
 class MachinePureOperatorTest : public TestWithZone {
  protected:
   MachineRepresentation word_type() {