Refine CountOperation of FullCodeGen

Refine CountOperation of FullCodeGen

[haitao.feng, 2013-08-13 09:05:56]

I'd like to know your feedback before porting it to IA32 and ARM.

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (left):

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc#o...
src/x64/full-codegen-x64.cc:4433: __ SmiAddConstant(rax, rax, Smi::FromInt(1));
It takes me some time to understand that the original code uses the
SmiAddConstant for both SMI and HeapNumber.

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc#o...
src/x64/full-codegen-x64.cc:4439: // the first smi check before calling
ToNumber.
This Cl splits the code at the first smi check.

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc#n...
src/x64/full-codegen-x64.cc:4425: if (expr->op() == Token::INC) {
An alternative is:
    if (expr->op() == Token::INC) {
      __ SmiAddConstant(rax, rax, Smi::FromInt(1), &stub_call);
    } else {
      __ SmiSubConstant(rax, rax, Smi::FromInt(1), &stub_call);
    }
    __ jmp(&done, Label::kNear);
    __ bind(&slow);

This version uses 5 instructions in SmiAddConstant (movq kScratchRegister,
kSmiConstantRegister; addq(kScratchRegister, rax); j(overflow, &stub_call);
movq(rax, kScratchRegister), jmp(&done)) than 2 (addq(rax,
kSmiConstantRegister), j(no_overflow, &done) for normal cases.

[danno, 2013-08-13 12:37:53]

Although I don't see anything wrong with this patch, I am unclear of the
motivation. Is there a clear performance win by eliminating the eliminating the
duplicate smi check? Have you tried to quantify the improvement and is it
measurable?

If not, I suspect that having more compact code rather is more important than
squeezing a cycle or two more out of a loop, especially if it will end up
Crankshafted anyway if it's a performance bottleneck.

[haitao.feng, 2013-08-13 13:27:53]

Sorry I should make the motivation clear. I am addressing your comment at
https://codereview.chromium.org/21014003/patch/17001/18010. When I used "the
passing the stub-call label always" approach for 31-bit SMI, I found 33 fails
for this part of code as the SmiAddConstant and SmiSubConstant in the code are
used for both SMI and HeapNumber. For 31-bit SMI, I did not check HeapNumber,
and the result will be signed extend to 64 bit if no overflow and this destroy
the HeapNumber. The main purpose of this CL is to make sure SmiAddConstant and
SmiSubConstant only operate on SMI, no HeapNumber. The original code is very
clever and utilizes several facts to make the inlined SMI operation minimal
inside a loop. So I tried hard to keep the origin spirit.

If having more compact code is more important, maybe we could remove the begging
JumpIfSmi as now ToNumberStub is a hydrogen stub, for SMI, the overhead is a
call than the current code.

[danno, 2013-08-19 19:36:58]

https://chromiumcodereview.appspot.com/22935005/diff/1/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/22935005/diff/1/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:4425: if (expr->op() == Token::INC) {
I think the shorter sequence is better without the scratch register in the case
that performance matters.

Have you considered making the SmiAddConstant/SmiSubConstant handle the overflow
case similarly to how this code does? e.g.

SmiAddConstant(...) {
addq(...)
Label local_done
j(no_overflow, &local_done);
subq(...)
jmp(&stub)
bind(&local_done);
}

Or do the extra branches destroy performance in this case? 

Perhaps you should leave the "fast" version for now and we'll tackle this
problem in the CL that introduces the SmiWrapper

On 2013/08/13 09:05:57, haitao.feng wrote:
> An alternative is:
>     if (expr->op() == Token::INC) {
>       __ SmiAddConstant(rax, rax, Smi::FromInt(1), &stub_call);
>     } else {
>       __ SmiSubConstant(rax, rax, Smi::FromInt(1), &stub_call);
>     }
>     __ jmp(&done, Label::kNear);
>     __ bind(&slow);
> 
> This version uses 5 instructions in SmiAddConstant (movq kScratchRegister,
> kSmiConstantRegister; addq(kScratchRegister, rax); j(overflow, &stub_call);
> movq(rax, kScratchRegister), jmp(&done)) than 2 (addq(rax,
> kSmiConstantRegister), j(no_overflow, &done) for normal cases.

[haitao.feng, 2013-08-20 14:36:05]

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc#n...
src/x64/full-codegen-x64.cc:4425: if (expr->op() == Token::INC) {
Thanks for the review and recommendation. I will make this CL an incremental
step of introducing the SmiWrapper.

How about the other part of this CL? i.e., guiding the SmiAddConstant and
SmiSubConstant by JumpIfNotSmi? Do we need to address the issue of using
SmiAddConstant for both SMI and HeapNumber?

[danno, 2013-08-20 15:39:37]

https://chromiumcodereview.appspot.com/22935005/diff/1/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/22935005/diff/1/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:4425: if (expr->op() == Token::INC) {
On 2013/08/20 14:36:05, haitao.feng wrote:
> Thanks for the review and recommendation. I will make this CL an incremental
> step of introducing the SmiWrapper.
> 
> How about the other part of this CL? i.e., guiding the SmiAddConstant and
> SmiSubConstant by JumpIfNotSmi? Do we need to address the issue of using
> SmiAddConstant for both SMI and HeapNumber?

I like your current approach where the Smi/no Smi decision is made very early,
that looks good. The "heap number optimization" in the old code was (as you
point out) subtle and it's unclear to me that the old version is actually a
performance win. It saved on code size, but without concrete performance numbers
to show that it was better your new, explicit split of smi/heap code number
early-on seems c cleaner and easier to understand.

I think there might even be a bug in the old code that you have fixed!
Previously, if there was an overflow in the smi constant add/sub case, we'd jump
to the stub... but not restore rax to it's pre-increment state. The stub would
work on the post-incremented value, not the pre-incremented value!

[haitao.feng, 2013-08-20 23:21:47]

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/22935005/diff/1/src/x64/full-codegen-x64.cc#n...
src/x64/full-codegen-x64.cc:4425: if (expr->op() == Token::INC) {
Thanks for the comments. Then I will port it to IA32 and ARM. Please note that
"Save result for postfix expressions" are duplicated in the code because of this
early split. I did not add a sub function in FullCodeGen for it as it is small.

There is no bug in the original code, the code after the label stub_call will
restore rax first and then call the stub function. This code is quite special
and different with any other SMI function usage.

[haitao.feng, 2013-10-01 03:18:42]

Danno, please have a look on the SmiSubConstant usage in the CountOperation for
X64.

For the SMI interface between full-codegen/stubs and crankshaft, I'd like to
use:

enum SmiExecutionMode {
  SMI_NO_CONSTRAINT = 0,                            // 0000000
  SMI_NEED_RESERVE_SOURCES = 0x1,                   // 0000001
  SMI_BAILOUT_ON_NO_OVERFLOW = 0x2,                 // 0000010
  SMI_BAILOUT_ON_OVERFLOW = 0x4,                    // 0000100
  SMI_BAILOUT_ON_MINUS_ZERO = 0x8                   // 0000100
}; 

to control the SMI function execution and use: 

class SmiOperationBailoutHandler {
 public:
  virtual void BailoutIf(Condition cc) const = 0;
}
to insert DeoptimizeIf/j into the SMI code sequence. The Condition cc could be
used to pass information.

I think this is also your recommendation from the code review of
https://codereview.chromium.org/21014003/. If you are OK with this, I will start
modifying/refactoring/merging the SMI marco instructions for X64.

[Sven Panne, 2013-10-10 10:46:44]

Quick DBC about a detail of this CL, didn't look very closely into the rest...

https://codereview.chromium.org/22935005/diff/13001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22935005/diff/13001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:56: enum SmiExecutionMode {
Don't use an enum here, which fundamentally wrong, because it mixes up the
notions of a set of elements and an individual element of such a set. Use the
EnumSet template instead, where you have a clear distinction.

[haitao.feng, 2013-10-11 12:09:46]

Sven, thanks for your recommendation. Comments addressed.

[danno, 2013-10-14 18:30:45]

Getting closer. Please also verify that there are no regressions on benchmarks
that are sensitive to full codegen, e.g. SunSpider

https://codereview.chromium.org/22935005/diff/19001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/22935005/diff/19001/src/x64/full-codegen-x64....
src/x64/full-codegen-x64.cc:4426: (1 << NEED_RESERVE_SOURCES) | (1 <<
BAILOUT_ON_NO_OVERFLOW));
I think this is clearer as:

SmiOperationExecutionMode mode;
mode.Add(NEED_RESERVE_SOURCES);
mode.Add(BAILOUT_ON_NO_OVERFLOW);

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:1594: UNIMPLEMENTED();  // Not used.
I would feel more comfortable if you implemented this now with tests, otherwise
we could get silent failures in the future in release builds.

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:1597: ASSERT(mode.IsEmpty());
Turn this ASSERT into a CHECK

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:1600: UNIMPLEMENTED();  // Not used.
I would feel more comfortable if you implemented this now with tests, otherwise
we could get silent failures in the future in release builds.

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:57: NEED_RESERVE_SOURCES,
This should be PRESERVE_SOURCE_REGISTER

[haitao.feng, 2013-10-16 09:17:01]

danno, thanks for the review. Comments addressed.

In my machine, running Sunspider with x64.release/d8,
** TOTAL **:           -                 147.5ms +/- 1.1%    147.2ms +/- 1.4%
and running Sunspider with ia32.release/d8,
** TOTAL **:           ??                150.1ms +/- 1.9%    150.4ms +/- 1.8%

https://codereview.chromium.org/22935005/diff/19001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/22935005/diff/19001/src/x64/full-codegen-x64....
src/x64/full-codegen-x64.cc:4426: (1 << NEED_RESERVE_SOURCES) | (1 <<
BAILOUT_ON_NO_OVERFLOW));
On 2013/10/14 18:30:45, danno wrote:
> I think this is clearer as:
> 
> SmiOperationExecutionMode mode;
> mode.Add(NEED_RESERVE_SOURCES);
> mode.Add(BAILOUT_ON_NO_OVERFLOW);

Done.

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:1594: UNIMPLEMENTED();  // Not used.
On 2013/10/14 18:30:45, danno wrote:
> I would feel more comfortable if you implemented this now with tests,
otherwise
> we could get silent failures in the future in release builds.

Done.

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:1597: ASSERT(mode.IsEmpty());
On 2013/10/14 18:30:45, danno wrote:
> Turn this ASSERT into a CHECK

Done.

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.cc:1600: UNIMPLEMENTED();  // Not used.
On 2013/10/14 18:30:45, danno wrote:
> I would feel more comfortable if you implemented this now with tests,
otherwise
> we could get silent failures in the future in release builds.

Done.

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22935005/diff/19001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:57: NEED_RESERVE_SOURCES,
On 2013/10/14 18:30:45, danno wrote:
> This should be PRESERVE_SOURCE_REGISTER

Done.

[danno, 2013-10-23 13:12:04]

This is getting pretty close to being landable. It would be even better and more
symmetrical if you also added the SmiOperationExecutionMode to SmiAddConstant as
well on x64. Since you will have to do this anyway, would you be willing to add
that machinery for SmiAddConstant in this CL, too?

https://codereview.chromium.org/22935005/diff/29001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22935005/diff/29001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:57: RESERVE_SOURCE_REGISTER,
This should be "PRESERVE_SOURCE_REGISTER" not "RESERVE_SOURCE_REGISTER"

[haitao.feng, 2013-10-24 12:30:45]

danno, thanks for the review. SmiAddConstant is uploaded.

https://codereview.chromium.org/22935005/diff/29001/src/x64/macro-assembler-x...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/22935005/diff/29001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:57: RESERVE_SOURCE_REGISTER,
On 2013/10/23 13:12:05, danno wrote:
> This should be "PRESERVE_SOURCE_REGISTER" not "RESERVE_SOURCE_REGISTER"

Done.

[danno, 2013-10-24 17:20:33]

It looks like the latest patch set did not upload correctly, so I can't review
the new code. Can you please delete the latest patch set and try again? Or in
the worst case upload into another CL.

Thanks

[haitao.feng, 2013-10-25 01:57:33]

Sorry about this. I did both. If you still could not review the update, I
created a copy of this CL at https://codereview.chromium.org/42973002/.