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

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

 // Copyright 2013 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/code-generator.h"
 
 #include "src/compilation-info.h"
 #include "src/compiler/code-generator-impl.h"
 #include "src/compiler/gap-resolver.h"
 #include "src/compiler/node-matchers.h"
@@ skipping 742 matching lines @@
     __ CallCFunction(ExternalReference::ieee754_##name##_function(isolate()), \
                      2);                                                      \
     /* Return value is in st(0) on ia32. */                                   \
     /* Store it into the result register. */                                  \
     __ sub(esp, Immediate(kDoubleSize));                                      \
     __ fstp_d(Operand(esp, 0));                                               \
     __ movsd(i.OutputDoubleRegister(), Operand(esp, 0));                      \
     __ 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);
 }
 
 void CodeGenerator::AssemblePrepareTailCall() {
   if (frame_access_state()->has_frame()) {
     __ mov(ebp, MemOperand(ebp, 0));
   }
   frame_access_state()->SetFrameAccessToSP();
@@ skipping 350 matching lines @@
     case kIeee754Float64Sinh:
       ASSEMBLE_IEEE754_UNOP(sinh);
       break;
     case kIeee754Float64Tan:
       ASSEMBLE_IEEE754_UNOP(tan);
       break;
     case kIeee754Float64Tanh:
       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);
       break;
     case kIA32Cmp16:
       ASSEMBLE_COMPARE(cmpw);
       break;
     case kIA32Cmp8:
       ASSEMBLE_COMPARE(cmpb);
       break;
@@ skipping 27 matching lines @@
       __ Move(edx, Immediate(0));
       __ div(i.InputOperand(1));
       break;
     case kIA32Not:
       __ not_(i.OutputOperand());
       break;
     case kIA32Neg:
       __ 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)) {
         __ shl(i.OutputOperand(), i.InputInt5(1));
       } else {
         __ shl_cl(i.OutputOperand());
       }
       break;
     case kIA32Shr:
       if (HasImmediateInput(instr, 1)) {
@@ skipping 1451 matching lines @@
     int padding_size = last_lazy_deopt_pc_ + space_needed - current_pc;
     __ Nop(padding_size);
   }
 }
 
 #undef __
 
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8