[wasm] Int64Lowering of Word64Ror and Word64Rol.

src/compiler/int64-lowering.cc

 // Copyright 2014 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.
 
 #include "src/compiler/int64-lowering.h"
 #include "src/compiler/common-operator.h"
 #include "src/compiler/diamond.h"
 #include "src/compiler/graph.h"
 #include "src/compiler/linkage.h"
 #include "src/compiler/machine-operator.h"
+#include "src/compiler/node-matchers.h"
 #include "src/compiler/node-properties.h"
 
 #include "src/compiler/node.h"
 #include "src/wasm/wasm-module.h"
 #include "src/zone.h"
 
 namespace v8 {
 namespace internal {
 namespace compiler {
 
@@ skipping 522 matching lines @@
                            graph()->NewNode(common()->Int32Constant(4)), store,
                            graph()->start());
 
       Node* low_node =
           graph()->NewNode(machine()->Load(MachineType::Int32()), stack_slot,
                            graph()->NewNode(common()->Int32Constant(0)), store,
                            graph()->start());
       ReplaceNode(node, low_node, high_node);
       break;
     }
+    case IrOpcode::kWord64Ror: {

[titzer, 2016/03/30 14:56:35]

The names in here are a bit confusing. Fixed have a variety of meanings.

Suggestions:

fixed_shift -> masked_shift (or even masked_shift5 to indicate 5 bits)
fixed_low -> low_input
fixed_high -> high_input

[ahaas, 2016/03/31 14:17:12]

Done.

+      DCHECK(node->InputCount() == 2);
+      Node* input = node->InputAt(0);
+      Node* shift = HasReplacementLow(node->InputAt(1))
+                        ? GetReplacementLow(node->InputAt(1))
+                        : node->InputAt(1);
+      Int32Matcher m(shift);
+      if (m.HasValue()) {
+        int32_t shift_value = m.Value() & 0x3f;
+
+        Node* fixed_low;
+        Node* fixed_high;
+        if (shift_value < 32) {
+          fixed_low = GetReplacementLow(input);
+          fixed_high = GetReplacementHigh(input);
+        } else {
+          fixed_low = GetReplacementHigh(input);
+          fixed_high = GetReplacementLow(input);
+        }
+
+        int32_t fixed_shift_value = shift_value & 0x1f;
+        Node* fixed_shift =
+            graph()->NewNode(common()->Int32Constant(fixed_shift_value));
+        Node* inv_shift =
+            graph()->NewNode(common()->Int32Constant(32 - fixed_shift_value));
+        if (fixed_shift_value == 0) {

[titzer, 2016/03/30 14:56:35]

Can you move this case up and save the creation of the constants just above? You
can make it even clearer if you just check shift_value == 32.

[ahaas, 2016/03/31 14:17:12]

Done.

+          ReplaceNode(node, fixed_low, fixed_high);
+        } else {
+          Node* low_node = graph()->NewNode(
+              machine()->Word32Or(),
+              graph()->NewNode(machine()->Word32Shr(), fixed_low, fixed_shift),
+              graph()->NewNode(machine()->Word32Shl(), fixed_high, inv_shift));
+          Node* high_node = graph()->NewNode(
+              machine()->Word32Or(),
+              graph()->NewNode(machine()->Word32Shr(), fixed_high, fixed_shift),
+              graph()->NewNode(machine()->Word32Shl(), fixed_low, inv_shift));
+          ReplaceNode(node, low_node, high_node);
+        }
+      } else {
+        Diamond lt32(graph(), common(),
+                     graph()->NewNode(
+                         machine()->Int32LessThan(),
+                         graph()->NewNode(
+                             machine()->Word32And(), shift,
+                             graph()->NewNode(common()->Int32Constant(0x3f))),
+                         graph()->NewNode(common()->Int32Constant(32))));
+
+        Node* fixed_low =

[titzer, 2016/03/30 14:56:35]

This is probably going to generate some bad code. The reason is that there is
essentially a conditional swap for both the high and low inputs (if shift >=
32).

A better sequence, which is probably less code with no swaps, is to do something
like

if(masked_shift < 32) (rotate code with non-swapped inputs)
else (rotate code with swapped inputs)

That way there are no extra moves involved with resolving the phis.

[ahaas, 2016/03/31 14:17:13]

I did a complete rework of the lowering. Word64Ror is now lowered to two
Word32Ror. This allows us to get rid of one diamond.

+            lt32.Phi(MachineRepresentation::kWord32, GetReplacementLow(input),
+                     GetReplacementHigh(input));
+        Node* fixed_high =
+            lt32.Phi(MachineRepresentation::kWord32, GetReplacementHigh(input),
+                     GetReplacementLow(input));
+
+        Node* fixed_shift =
+            graph()->NewNode(machine()->Word32And(), shift,
+                             graph()->NewNode(common()->Int32Constant(0x1f)));
+        Node* inv_shift = graph()->NewNode(
+            machine()->Int32Sub(),
+            graph()->NewNode(common()->Int32Constant(32)), fixed_shift);
+
+        Diamond z(
+            graph(), common(),
+            graph()->NewNode(machine()->Word32Equal(), fixed_shift,
+                             graph()->NewNode(common()->Int32Constant(0))));
+
+        Node* low_node =
+            z.Phi(MachineRepresentation::kWord32, fixed_low,
+                  graph()->NewNode(machine()->Word32Or(),
+                                   graph()->NewNode(machine()->Word32Shr(),
+                                                    fixed_low, fixed_shift),
+                                   graph()->NewNode(machine()->Word32Shl(),
+                                                    fixed_high, inv_shift)));
+
+        Node* high_node =
+            z.Phi(MachineRepresentation::kWord32, fixed_high,
+                  graph()->NewNode(machine()->Word32Or(),
+                                   graph()->NewNode(machine()->Word32Shr(),
+                                                    fixed_high, fixed_shift),
+                                   graph()->NewNode(machine()->Word32Shl(),
+                                                    fixed_low, inv_shift)));
+
+        ReplaceNode(node, low_node, high_node);
+      }
+      break;
+    }
     // kExprI64Clz:
     case IrOpcode::kWord64Clz: {
       DCHECK(node->InputCount() == 1);
       Node* input = node->InputAt(0);
       Diamond d(
           graph(), common(),
           graph()->NewNode(machine()->Word32Equal(), GetReplacementHigh(input),
                            graph()->NewNode(common()->Int32Constant(0))));
 
       Node* low_node = d.Phi(
@@ skipping 54 matching lines @@
           high_node->ReplaceInput(i, GetReplacementHigh(node->InputAt(i)));
         }
       } else {
         DefaultLowering(node);
       }
       break;
     }
 
     default: { DefaultLowering(node); }
   }
-}
+}  // NOLINT(readability/fn_size)
 
 void Int64Lowering::LowerComparison(Node* node, const Operator* high_word_op,
                                     const Operator* low_word_op) {
   DCHECK(node->InputCount() == 2);
   Node* left = node->InputAt(0);
   Node* right = node->InputAt(1);
   Node* replacement = graph()->NewNode(
       machine()->Word32Or(),
       graph()->NewNode(high_word_op, GetReplacementHigh(left),
                        GetReplacementHigh(right)),
@@ skipping 71 matching lines @@
                     common()->Phi(MachineRepresentation::kWord32, value_count),
                     value_count + 1, inputs_low, false),
                 graph()->NewNode(
                     common()->Phi(MachineRepresentation::kWord32, value_count),
                     value_count + 1, inputs_high, false));
   }
 }
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8