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

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

With this change, on ia32 and x64, a load from memory into a register can be replaced by a memory operand for integer binops if it makes sense.

BUG=

Review-Url: https://codereview.chromium.org/2728533003
Cr-Commit-Position: refs/heads/master@{#43739}
Committed: https://chromium.googlesource.com/v8/v8/+/78199ce5e0f3f6597284e0dae25164ccd8e63886

[shiyu.zhang, 2017-03-09 03:17:26]

Description was changed from

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



BUG=
==========

to

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

With this change, on ia32 and x64, a load from memory into a register can be
replaced by a memory operand for integer binops if it makes sense.

BUG=
==========

[shiyu.zhang, 2017-03-09 05:36:10]

shiyu.zhang@intel.com changed reviewers:
+ bmeurer@chromium.org, danno@chromium.org

[shiyu.zhang, 2017-03-09 05:37:39]

On 2017/03/09 05:36:10, shiyu.zhang wrote:
> mailto:shiyu.zhang@intel.com changed reviewers:
> + mailto:bmeurer@chromium.org, mailto:danno@chromium.org

PTAL.

[danno, 2017-03-09 06:26:09]

danno@chromium.org changed reviewers:
+ ahaas@chromium.org
- bmeurer@chromium.org

[danno, 2017-03-09 06:26:09]

Andreas, could you perhaps take a look?

[ahaas, 2017-03-09 07:10:59]

Can you add tests for this CL, e.g. in instruction-selector-ia32-unittests.cc?

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:763: #define ASSEMBLE_BINOP(asm_instr) 
                                   \
Is it worth to implement ASSEMBLE_BINOP as a macro in terms of performance, or
could it just be implemented as a normal function?

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:765: if
(AddressingModeField::decode(instr->opcode()) != kMode_None) { \
Can you pass in {instr} as a parameter to the macro?

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

Hi ahaas, thanks for the comments! I am working on the tests.

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:763: #define ASSEMBLE_BINOP(asm_instr) 
                                   \
On 2017/03/09 07:10:58, ahaas wrote:
> Is it worth to implement ASSEMBLE_BINOP as a macro in terms of performance, or
> could it just be implemented as a normal function?

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?

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:765: if
(AddressingModeField::decode(instr->opcode()) != kMode_None) { \
On 2017/03/09 07:10:58, ahaas wrote:
> Can you pass in {instr} as a parameter to the macro? 

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

[shiyu.zhang, 2017-03-10 10:53:35]

On 2017/03/09 09:53:17, shiyu.zhang wrote:
> Hi ahaas, thanks for the comments! I am working on the tests.
> 
>
https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
> File src/compiler/ia32/code-generator-ia32.cc (right):
> 
>
https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
> src/compiler/ia32/code-generator-ia32.cc:763: #define
ASSEMBLE_BINOP(asm_instr) 
>                                    \
> On 2017/03/09 07:10:58, ahaas wrote:
> > Is it worth to implement ASSEMBLE_BINOP as a macro in terms of performance,
or
> > could it just be implemented as a normal function?
> 
> 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?
> 
>
https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
> src/compiler/ia32/code-generator-ia32.cc:765: if
> (AddressingModeField::decode(instr->opcode()) != kMode_None) { \
> On 2017/03/09 07:10:58, ahaas wrote:
> > Can you pass in {instr} as a parameter to the macro? 
> 
> The usage of ASSEMBLE_BINOP is similar to ASSEMBLE_COMPARE in this file. Is it
> really necessary to pass in {instr} as a parameter?

Hi ahaas, I have added some tests for this CL. Could you please take a look?

Besides, I found the instruction-selector matched most of add to a lea pattern.
It makes some possible and reasonable add-with-memory-operand patterns invalid.
I am thinking about solving it in next CL after this CL landed. Do you have any
suggestion on this?

Appreciate your comments, thanks!

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

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:763: #define ASSEMBLE_BINOP(asm_instr) 
                                   \
On 2017/03/09 at 09:53:16, shiyu.zhang wrote:
> On 2017/03/09 07:10:58, ahaas wrote:
> > Is it worth to implement ASSEMBLE_BINOP as a macro in terms of performance,
or
> > could it just be implemented as a normal function?
> 
> 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?

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.

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:765: if
(AddressingModeField::decode(instr->opcode()) != kMode_None) { \
On 2017/03/09 at 09:53:17, shiyu.zhang wrote:
> On 2017/03/09 07:10:58, ahaas wrote:
> > Can you pass in {instr} as a parameter to the macro? 
> 
> The usage of ASSEMBLE_BINOP is similar to ASSEMBLE_COMPARE in this file. Is it
really necessary to pass in {instr} as a parameter?

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?

https://codereview.chromium.org/2728533003/diff/40001/test/unittests/compiler...
File test/unittests/compiler/x64/instruction-selector-x64-unittest.cc (right):

https://codereview.chromium.org/2728533003/diff/40001/test/unittests/compiler...
test/unittests/compiler/x64/instruction-selector-x64-unittest.cc:1212:
TEST_F(InstructionSelectorTest, LoadAnd32) {
Nit: can you also add the tests for the 64-bit variants?

[ahaas, 2017-03-10 12:05:47]

On 2017/03/10 at 10:53:35, shiyu.zhang wrote:
> On 2017/03/09 09:53:17, shiyu.zhang wrote:
> > Hi ahaas, thanks for the comments! I am working on the tests.
> > 
> >
https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
> > File src/compiler/ia32/code-generator-ia32.cc (right):
> > 
> >
https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
> > src/compiler/ia32/code-generator-ia32.cc:763: #define
ASSEMBLE_BINOP(asm_instr) 
> >                                    \
> > On 2017/03/09 07:10:58, ahaas wrote:
> > > Is it worth to implement ASSEMBLE_BINOP as a macro in terms of
performance, or
> > > could it just be implemented as a normal function?
> > 
> > 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?
> > 
> >
https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
> > src/compiler/ia32/code-generator-ia32.cc:765: if
> > (AddressingModeField::decode(instr->opcode()) != kMode_None) { \
> > On 2017/03/09 07:10:58, ahaas wrote:
> > > Can you pass in {instr} as a parameter to the macro? 
> > 
> > The usage of ASSEMBLE_BINOP is similar to ASSEMBLE_COMPARE in this file. Is
it
> > really necessary to pass in {instr} as a parameter?
> 
> Hi ahaas, I have added some tests for this CL. Could you please take a look?
> 
> Besides, I found the instruction-selector matched most of add to a lea
pattern. It makes some possible and reasonable add-with-memory-operand patterns
invalid. I am thinking about solving it in next CL after this CL landed. Do you
have any suggestion on this?

I think it is a good idea to do this in a separate CL. 

> 
> Appreciate your comments, thanks!

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

Hi ahaas, tests for 64-bit variants are added. Could you please take a look?

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:765: if
(AddressingModeField::decode(instr->opcode()) != kMode_None) { \
On 2017/03/10 12:02:50, ahaas wrote:
> On 2017/03/09 at 09:53:17, shiyu.zhang wrote:
> > On 2017/03/09 07:10:58, ahaas wrote:
> > > Can you pass in {instr} as a parameter to the macro? 
> > 
> > The usage of ASSEMBLE_BINOP is similar to ASSEMBLE_COMPARE in this file. Is
it
> really necessary to pass in {instr} as a parameter?
> 
> 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?

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?

https://codereview.chromium.org/2728533003/diff/40001/test/unittests/compiler...
File test/unittests/compiler/x64/instruction-selector-x64-unittest.cc (right):

https://codereview.chromium.org/2728533003/diff/40001/test/unittests/compiler...
test/unittests/compiler/x64/instruction-selector-x64-unittest.cc:1212:
TEST_F(InstructionSelectorTest, LoadAnd32) {
On 2017/03/10 12:02:50, ahaas wrote:
> Nit: can you also add the tests for the 64-bit variants?

Done.

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

lgtm.

danno, can you lgtm as well?

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2728533003/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:765: if
(AddressingModeField::decode(instr->opcode()) != kMode_None) { \
On 2017/03/13 at 02:39:35, shiyu.zhang wrote:
> On 2017/03/10 12:02:50, ahaas wrote:
> > On 2017/03/09 at 09:53:17, shiyu.zhang wrote:
> > > On 2017/03/09 07:10:58, ahaas wrote:
> > > > Can you pass in {instr} as a parameter to the macro? 
> > > 
> > > The usage of ASSEMBLE_BINOP is similar to ASSEMBLE_COMPARE in this file.
Is it
> > really necessary to pass in {instr} as a parameter?
> > 
> > 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?
> 
> 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?

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

[Benedikt Meurer, 2017-03-13 08:14:56]

bmeurer@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2017-03-13 08:14:57]

LGTM, very nice! Thanks.

[shiyu.zhang, 2017-03-13 08:49:07]

The CQ bit was checked by shiyu.zhang@intel.com

[commit-bot: I haz the power, 2017-03-13 08:49:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 08:50:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489394947555810,
"parent_rev": "4aeb26579993622ff374cb21dbfcfa2ba2d673d8", "commit_rev":
"78199ce5e0f3f6597284e0dae25164ccd8e63886"}

[commit-bot: I haz the power, 2017-03-13 08:50:59]

Description was changed from

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

With this change, on ia32 and x64, a load from memory into a register can be
replaced by a memory operand for integer binops if it makes sense.

BUG=
==========

to

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

With this change, on ia32 and x64, a load from memory into a register can be
replaced by a memory operand for integer binops if it makes sense.

BUG=

Review-Url: https://codereview.chromium.org/2728533003
Cr-Commit-Position: refs/heads/master@{#43739}
Committed:
https://chromium.googlesource.com/v8/v8/+/78199ce5e0f3f6597284e0dae25164ccd8e...
==========

[commit-bot: I haz the power, 2017-03-13 08:51:00]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/78199ce5e0f3f6597284e0dae25164ccd8e...