[wasm] Int64Lowering of Word64Ror and Word64Rol.

src/compiler/int64-lowering.cc

Index: src/compiler/int64-lowering.cc
diff --git a/src/compiler/int64-lowering.cc b/src/compiler/int64-lowering.cc
index 79ffec61fd7b113b10f4cb2833c04db505168dd0..ea023375543b002ef372d0aa9502bcf824cc70f1 100644
--- a/src/compiler/int64-lowering.cc
+++ b/src/compiler/int64-lowering.cc
@@ -8,6 +8,7 @@
 #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"
@@ -550,6 +551,92 @@ void Int64Lowering::LowerNode(Node* node) {
       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);
@@ -624,7 +711,7 @@ void Int64Lowering::LowerNode(Node* node) {
 
     default: { DefaultLowering(node); }
   }
-}
+}  // NOLINT(readability/fn_size)
 
 void Int64Lowering::LowerComparison(Node* node, const Operator* high_word_op,
                                     const Operator* low_word_op) {