[turbofan] Enable complex memory operands for binops on ia32/x64

src/compiler/ia32/code-generator-ia32.cc

Index: src/compiler/ia32/code-generator-ia32.cc
diff --git a/src/compiler/ia32/code-generator-ia32.cc b/src/compiler/ia32/code-generator-ia32.cc
index 0f6d1c3a4a8ff7db2ed2a07436e3a96e542bbe16..dcfa898bf253154f47eeef3915e20a80f6173895 100644
--- a/src/compiler/ia32/code-generator-ia32.cc
+++ b/src/compiler/ia32/code-generator-ia32.cc
@@ -760,6 +760,21 @@ class OutOfLineRecordWrite final : public OutOfLineCode {
     __ add(esp, Immediate(kDoubleSize));                                      \
   } while (false)
 
+#define ASSEMBLE_BINOP(asm_instr)                                     \

[ahaas, 2017/03/09 07:10:58]

Is it worth to implement ASSEMBLE_BINOP as a macro in terms of performance, or
could it just be implemented as a normal function?

[shiyu.zhang, 2017/03/09 09:53:16]

I use a macro here in order to be consistent with x64. And similar macro usage
(like "ASSEMBLE_COMPARE") can be seen in this file. I think using a macro will
not hurt the runtime performance since it just does a replacing, right? Do I
really need to implement it as a normal function?

[ahaas, 2017/03/10 12:02:50]

I think it's fine like this for this CL, since it is consistent with the rest of
the file. It would be interesting in general though if the use of macros here
pays off.

+  do {                                                                \
+    if (AddressingModeField::decode(instr->opcode()) != kMode_None) { \

[ahaas, 2017/03/09 07:10:58]

Can you pass in {instr} as a parameter to the macro? 

[shiyu.zhang, 2017/03/09 09:53:17]

The usage of ASSEMBLE_BINOP is similar to ASSEMBLE_COMPARE in this file. Is it
really necessary to pass in {instr} as a parameter?

[ahaas, 2017/03/10 12:02:50]

I think it's a bad style to assume implicitly in the macro that there exists an
{instr} parameter, especially since the macro is not defined within the function
that uses it. However, if we changed it here, we would have to change it in all
macros. Could you do this change in a separate CL?

[shiyu.zhang, 2017/03/13 02:39:35]

Sure. And I think {i} should also be passed in as a parameter to the macro,
right? Besides, all the similar macros for arm/arm64, mips/mips64, ppc, s390
maybe also need to be changed. Would you like me to do the change for them all?

[ahaas, 2017/03/13 08:10:57]

Oh, I was not aware that this code style is all over the place. Never mind in
that case.

+      size_t index = 1;                                               \
+      Operand right = i.MemoryOperand(&index);                        \
+      __ asm_instr(i.InputRegister(0), right);                        \
+    } else {                                                          \
+      if (HasImmediateInput(instr, 1)) {                              \
+        __ asm_instr(i.InputOperand(0), i.InputImmediate(1));         \
+      } else {                                                        \
+        __ asm_instr(i.InputRegister(0), i.InputOperand(1));          \
+      }                                                               \
+    }                                                                 \
+  } while (0)
+
 void CodeGenerator::AssembleDeconstructFrame() {
   __ mov(esp, ebp);
   __ pop(ebp);
@@ -1130,18 +1145,10 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
       ASSEMBLE_IEEE754_UNOP(tanh);
       break;
     case kIA32Add:
-      if (HasImmediateInput(instr, 1)) {
-        __ add(i.InputOperand(0), i.InputImmediate(1));
-      } else {
-        __ add(i.InputRegister(0), i.InputOperand(1));
-      }
+      ASSEMBLE_BINOP(add);
       break;
     case kIA32And:
-      if (HasImmediateInput(instr, 1)) {
-        __ and_(i.InputOperand(0), i.InputImmediate(1));
-      } else {
-        __ and_(i.InputRegister(0), i.InputOperand(1));
-      }
+      ASSEMBLE_BINOP(and_);
       break;
     case kIA32Cmp:
       ASSEMBLE_COMPARE(cmp);
@@ -1189,25 +1196,13 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
       __ neg(i.OutputOperand());
       break;
     case kIA32Or:
-      if (HasImmediateInput(instr, 1)) {
-        __ or_(i.InputOperand(0), i.InputImmediate(1));
-      } else {
-        __ or_(i.InputRegister(0), i.InputOperand(1));
-      }
+      ASSEMBLE_BINOP(or_);
       break;
     case kIA32Xor:
-      if (HasImmediateInput(instr, 1)) {
-        __ xor_(i.InputOperand(0), i.InputImmediate(1));
-      } else {
-        __ xor_(i.InputRegister(0), i.InputOperand(1));
-      }
+      ASSEMBLE_BINOP(xor_);
       break;
     case kIA32Sub:
-      if (HasImmediateInput(instr, 1)) {
-        __ sub(i.InputOperand(0), i.InputImmediate(1));
-      } else {
-        __ sub(i.InputRegister(0), i.InputOperand(1));
-      }
+      ASSEMBLE_BINOP(sub);
       break;
     case kIA32Shl:
       if (HasImmediateInput(instr, 1)) {