Optionally use 31-bits SMI value for 64-bit system

Optionally use 31-bits SMI value for 64-bit system

[haitao.feng, 2013-07-29 07:44:37]

I'd like to get your high-level feedback on this CL. The original purpose is to
minimize the code difference of X64 and X32 port. We could get the X32 SMI part
by removing the movsxlq in this CL.

Comparing with 32-bit SMI value, 31-bits SMI value has some advantages:
   1) SMI register might be used as index register directly inside SIB
when the scale is 2, 4, 8. In this case, just change the scale to
orig_scale >> 1. (This is done in the full codegen, I have not looked at into
hydrogen for this opportunity)
  2) SMI value could be used as Immediate and embedded into instruction as
immediate is signed extend into 64-bit. And we could free kSmiConstantRegister
for register allocation or other usage.

I have gotten 100% pass with make x64.[release|debug].check
use_31_bits_smi_value=on -j8. And the 31-bit SMI performance is on par with
32-bit SMI on my machine.

Bleeding_edge:                                                                  
                                                                                
Richards: 29654                                                                 
DeltaBlue: 29191                                                                
Crypto: 24250                                                                   
RayTrace: 43585                                                                 
EarleyBoyer: 38074                                                              
RegExp: 5581                                                                    
Splay: 10266                                                                    
NavierStokes: 26834                                                             
----                                                                            
Score (version 7): 21933                                                        
                                                                                
31-bit-smi:                                                                     
                                                                                
Richards: 30236                                                                 
DeltaBlue: 29416                                                                
Crypto: 23341                                                                   
RayTrace: 43024                                                                 
EarleyBoyer: 38041                                                              
RegExp: 5637                                                                    
Splay: 10225                                                                    
NavierStokes: 26834                                                             
----                                                                            
Score (version 7): 21881

[danno, 2013-07-29 13:02:23]

Thanks for the patch.

Although I don't have a fundamental problem with supporting both 31-bit and
32-bit smis on x64 to prepare for x32, this CL really complicates the code.
There are simply too many #ifdefs.

Consider the following to simplify and unify the code paths and remove #ifs:
- Move functionality into the macro assembler that needs to distinguish between
31/32 bit smis so that the calling code doesn't need #ifs, e.g. imul_smi() or
add_smi()...
- Extract common functionality that apply to smis into new macro assembler
functions that wrap their functionality
- In the macro assembler, when you absolutely have to distinguish between 31 and
32 bit smis, use "if (V8_USE_31_BITS_SMI_VALUE)" rather than "#if
V8_USE_31_BITS_SMI_VALUE". This makes sure the code is always "compilable" not
matter if the 31 bit smi switch is on or off.
- For stubs and other utility functions, augment the API to include all the
parameters you will need in all cases, even if they are not used. It simplifies
the interface and makes sure that there are fewer #ifs in the header files.

If the above isn't sufficient and there is no other way to drastically reduce
the number of #ifs, then I would prefer not going down this route and having the
x32 port support 31-bit smis separately than x64.

Please not that I didn't review the whole CL since I'd like my high-level
feedback addressed before spending to much time on the details.

https://codereview.chromium.org/21014003/diff/1/build/features.gypi
File build/features.gypi (right):

https://codereview.chromium.org/21014003/diff/1/build/features.gypi#newcode44
build/features.gypi:44: 'v8_use_31_bits_smi_value%': 0,
Call these 

v8_use_31_bit_smi_value
V8_USE_31_BIT_SMI_VALUE

https://codereview.chromium.org/21014003/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/21014003/diff/1/include/v8.h#newcode5350
include/v8.h:5350: #if !V8_USE_31_BITS_SMI_VALUE
I also see no reason to put the SmiTagging<8> definition in a #ifdef

Also, reverse the sense of the #ifdef


#if V8_USE_31_BIT_SMI_VALUE
typedef SmiTagging<4> PlatformSmiTagging;
#else
typedef SmiTagging<kApiPointerSize> PlatformSmiTagging;
#endif

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.cc#newc...
src/x64/assembler-x64.cc:3032: #if V8_USE_31_BITS_SMI_VALUE
No conditional #ifdefs in assembler

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newco...
src/x64/assembler-x64.h:382: explicit Immediate(Smi* value) {
Don't add a Smi* constructor here. The wrapper function that allows inlining of
31-bit smis should do the work that this constructor does and call through to
the existing version.

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newco...
src/x64/assembler-x64.h:996: #if V8_USE_31_BITS_SMI_VALUE
Is there some way to not including this conditionally? I think the assembler
should have no conditionally included code, it should be as "dumb" as possible
and not know anything about Smis if possible.

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newco...
src/x64/assembler-x64.h:1393: void pcmpeqd(XMMRegister dst, XMMRegister src);
Why can't this always be part of the code (no conditional include)?

https://codereview.chromium.org/21014003/diff/1/src/x64/disasm-x64.cc
File src/x64/disasm-x64.cc (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/disasm-x64.cc#newcode...
src/x64/disasm-x64.cc:1099: #endif
This doesn't belong in this CL

https://codereview.chromium.org/21014003/diff/1/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/lithium-codegen-x64.h...
src/x64/lithium-codegen-x64.h:125: void DoDeferredNumberTagI(LNumberTagI*
instr);
Please don't conditionally declare stuff unless it's absolutely necessary, is
there any hurt to always having this thing present, even if it's not used?

[haitao.feng, 2013-07-30 08:56:43]

danno, thanks a lot for the review. It is much cleaner than patchset 1.

https://codereview.chromium.org/21014003/diff/1/build/features.gypi
File build/features.gypi (right):

https://codereview.chromium.org/21014003/diff/1/build/features.gypi#newcode44
build/features.gypi:44: 'v8_use_31_bits_smi_value%': 0,
On 2013/07/29 13:02:23, danno wrote:
> Call these 
> 
> v8_use_31_bit_smi_value
> V8_USE_31_BIT_SMI_VALUE

Done.

https://codereview.chromium.org/21014003/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/21014003/diff/1/include/v8.h#newcode5350
include/v8.h:5350: #if !V8_USE_31_BITS_SMI_VALUE
On 2013/07/29 13:02:23, danno wrote:
> I also see no reason to put the SmiTagging<8> definition in a #ifdef
> 
> Also, reverse the sense of the #ifdef
> 
> 
> #if V8_USE_31_BIT_SMI_VALUE
> typedef SmiTagging<4> PlatformSmiTagging;
> #else
> typedef SmiTagging<kApiPointerSize> PlatformSmiTagging;
> #endif
> 
> 

Done.

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.cc
File src/x64/assembler-x64.cc (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.cc#newc...
src/x64/assembler-x64.cc:3032: #if V8_USE_31_BITS_SMI_VALUE
On 2013/07/29 13:02:23, danno wrote:
> No conditional #ifdefs in assembler

Done.

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newco...
src/x64/assembler-x64.h:382: explicit Immediate(Smi* value) {
On 2013/07/29 13:02:23, danno wrote:
> Don't add a Smi* constructor here. The wrapper function that allows inlining
of
> 31-bit smis should do the work that this constructor does and call through to
> the existing version.

Done.

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newco...
src/x64/assembler-x64.h:996: #if V8_USE_31_BITS_SMI_VALUE
On 2013/07/29 13:02:23, danno wrote:
> Is there some way to not including this conditionally? I think the assembler
> should have no conditionally included code, it should be as "dumb" as possible
> and not know anything about Smis if possible. 

Done.

https://codereview.chromium.org/21014003/diff/1/src/x64/assembler-x64.h#newco...
src/x64/assembler-x64.h:1393: void pcmpeqd(XMMRegister dst, XMMRegister src);
On 2013/07/29 13:02:23, danno wrote:
> Why can't this always be part of the code (no conditional include)?

Done.

https://codereview.chromium.org/21014003/diff/1/src/x64/disasm-x64.cc
File src/x64/disasm-x64.cc (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/disasm-x64.cc#newcode...
src/x64/disasm-x64.cc:1099: #endif
Removed the #ifdef

https://codereview.chromium.org/21014003/diff/1/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):

https://codereview.chromium.org/21014003/diff/1/src/x64/lithium-codegen-x64.h...
src/x64/lithium-codegen-x64.h:125: void DoDeferredNumberTagI(LNumberTagI*
instr);
On 2013/07/29 13:02:23, danno wrote:
> Please don't conditionally declare stuff unless it's absolutely necessary, is
> there any hurt to always having this thing present, even if it's not used?

Done.

[haitao.feng, 2013-08-01 03:38:04]

danno, thanks a lot for your review and recommendations. I think this CL is
ready for the second round review.

[danno, 2013-08-01 16:45:40]

This seems to be heading in the right direction, but there is a lot more work to
do... this is a first round of comments but there will be more to come as I make
more progress on the review

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:733: (kSmiValueSize == 31 && op == Token::SHL) ||
Is think you should turn this into a predicate:

SmisAreUint32()

that does this test.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:797: if (kSmiValueSize == 31) {
Why can't you put this extra stack manipulation in the 31-bit case inside the
SmiShiftLeft macro assembler instruction??

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:814: if (kSmiValueSize == 31) {
Same here.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:832: if (use_fp_on_smis.is_linked()) {
All of this use_fp_on_smis code can be encapsulated in the macro assembler,
can't it? You shouldn't even have to pass use_fp_on_smi as a label, only the
fail label, since the fp-handling case only needs to jump to that code if it
overflow the smi range. I know that's a bigger change, but if you just pass
generate_inline_heapnumber_results to the macro assembler instructions for all
of the math operations and the fail label, I think you can handle everything
there.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:887: default: UNREACHABLE();
Also, I think you can share much of this code with the version in the 32-bit
version. Only the stack pop restore at the end needs to be added in the 31-bit
case.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:942: static void
BinaryOpStub_GenerateFloatingPointCode(MacroAssembler* masm,
start the parameter list on the next line with a 4-char indentations, it saves
you the weird formatting of result_type below.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:1241: op_, result_type_, &not_int32, mode_);
strange indentation

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4766: __ SmiAdd(rbx, rbx, rcx);
As noted above, I think you should change SmiAdd to handle the 31-bit cases in
the macro-assmebler and have a single interface

https://codereview.chromium.org/21014003/diff/17001/src/x64/debug-x64.cc
File src/x64/debug-x64.cc (right):

https://codereview.chromium.org/21014003/diff/17001/src/x64/debug-x64.cc#newc...
src/x64/debug-x64.cc:129: __ movq(kScratchRegister, reg);
Watch out. You have change the semantic here slightly, since 64-bits can be
encoded in two 31-bit values.  However, in practice, all the uses of this code
actually only expect integers (32 bits) to be encoded, so I this you can change
the semantic to only pack the 32 lower bits sign extended.

I think this code sequence (and the restore sequence in this file) should be
moved into the macro assembler:

__ PushInt32AsTwoSmis(reg, kScratchRegister)
__ PopInt32AsTwoSmis(reg, kScratchRegister);

and then hide the smi size check in the macro assembler.

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

https://codereview.chromium.org/21014003/diff/17001/src/x64/full-codegen-x64....
src/x64/full-codegen-x64.cc:4456: __ SmiAddConstant(rax, rax, Smi::FromInt(1));
Handle the overflow case inside the macro assembler, passing the stub-call label
always

https://codereview.chromium.org/21014003/diff/17001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/21014003/diff/17001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1314: __ imul(left, ToOperand(right));
Move this switching into the macro assembler, you will need to introduce __
SmiIMul, etc.

[haitao.feng, 2013-08-02 09:35:51]

danno, thanks a lot for your review. I am glad to see I have anticipated some of
your comments and already addressed them in patch set 5 :)

I have uploaded the patch set 6, I will rebase with master this weekend and deal
with the SHL operation in hydrogen and address your other comment in hydrogen.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:733: (kSmiValueSize == 31 && op == Token::SHL) ||
Done. Add IsUnsafeSmiOperator function in the macro assembler.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:797: if (kSmiValueSize == 31) {
On 2013/08/01 16:45:41, danno wrote:
> Why can't you put this extra stack manipulation in the 31-bit case inside the
> SmiShiftLeft macro assembler instruction??

Done.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:814: if (kSmiValueSize == 31) {
On 2013/08/01 16:45:41, danno wrote:
> Same here.

Done.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:832: if (use_fp_on_smis.is_linked()) {
The X64 implementation utilizes a fact that when SHR 0 overflow, i.e., a
negative has to be represented as positive, the binary representation are the
same. If we do not use this optimization (we need to add an overhead of
movq(dst, src1) , I could get the code logic nearly the same for 31-bit SMI and
32-bit SMI. 

On 2013/08/01 16:45:41, danno wrote:
> All of this use_fp_on_smis code can be encapsulated in the macro assembler,
> can't it? You shouldn't even have to pass use_fp_on_smi as a label, only the
> fail label, since the fp-handling case only needs to jump to that code if it
> overflow the smi range. I know that's a bigger change, but if you just pass
> generate_inline_heapnumber_results to the macro assembler instructions for all
> of the math operations and the fail label, I think you can handle everything
> there.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:887: default: UNREACHABLE();
On 2013/08/01 16:45:41, danno wrote:
> Also, I think you can share much of this code with the version in the 32-bit
> version. Only the stack pop restore at the end needs to be added in the 31-bit
> case.

Done.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:942: static void
BinaryOpStub_GenerateFloatingPointCode(MacroAssembler* masm,
On 2013/08/01 16:45:41, danno wrote:
> start the parameter list on the next line with a 4-char indentations, it saves
> you the weird formatting of result_type below.

Done.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:1241: op_, result_type_, &not_int32, mode_);
On 2013/08/01 16:45:41, danno wrote:
> strange indentation

Done.

https://codereview.chromium.org/21014003/diff/17001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4766: __ SmiAdd(rbx, rbx, rcx);
There are two SmiAdd interfaces. SmiAdd (with no fail) is used only when it's
known that overflowing is impossible. I have handled the interface with fail in
macro-assembler. Here it is safe to use __ SmiAdd(rbx, rbx, rcx, &call_runtime),
but we have un-necessary runtime overhead for the 32-bit SMI.

https://codereview.chromium.org/21014003/diff/17001/src/x64/debug-x64.cc
File src/x64/debug-x64.cc (right):

https://codereview.chromium.org/21014003/diff/17001/src/x64/debug-x64.cc#newc...
src/x64/debug-x64.cc:129: __ movq(kScratchRegister, reg);
Done. I used the name PushInt64AsTwoSmis and PopInt64AsTwoSmis.

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

https://codereview.chromium.org/21014003/diff/17001/src/x64/full-codegen-x64....
src/x64/full-codegen-x64.cc:4456: __ SmiAddConstant(rax, rax, Smi::FromInt(1));
I am OK to change this but this might affect performance. For the SmiAddConstant
with overflow checking, we will movq(kScratchRegister, rax);
add(kScratchRegister, LoadSmiRegister(1)); j(overflow, fail, near_jump);
movq(rax, kScratchRegister). We need 4 instructions in the normal case than 2
(add, check overflow) inside a loop.

[haitao.feng, 2013-08-06 07:58:56]

I rebased with master and ported the Shl for 31-bit-SMI.  I introduced a class
SmiFunctionInvoker to abstract the difference between the FullCodeGen and
LCodeGen. I used it for SmiShiftLeftConstant. I'd like to know your high-level
feedback before applying it to the other Smi functions (Add, Sub, Mul).

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:3529: void LCodeGen::EmitSmiMathAbs(LMathAbs*
instr) {
Please ignore this part as Weiliang submitted a CL at
https://codereview.chromium.org/22285003/. I include them into this CL as they
get math-abs test case pass for 31 bit smi.

[danno, 2013-08-07 18:41:26]

I like the fundamental concept of the SmiFunctionInvoker, but I think you can
simplify it even more (see my comments to that effect where the class is
defined), e.g. I think it should only have one virtual method (except for the
destructor)

Also, everywhere, please introduce SmiValuesAre31Bits()/SmitValuesAre32Bits() as
a global predicate instead of having to write "kSmiValueSize == 31" everywhere.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:904: if (result_type <= BinaryOpIC::INT32) {
Why is UNITIALIZED also included? It's much clearer if you explicitly test for
the cases that are positive. I don't understand what this code is for. Are you
basically checking that the result will fit into a smi? But you are only
checking in the 31-bit smi case to see if the value is a 32-bit integer, that
confuses me. A comment would really help.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:905: __ cvttsd2si(kScratchRegister, xmm0);
Don't you need to check that bit 30 and 31 of scratch register are the same?
Otherwise tue result won't fit into a smi.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:910: __ j(zero, non_int32_failure);
Again, why is this non_int32_failure and not non_int31_failure? It would seem
that having a 32-bit number is equally important in the case that smis are
32-bits.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:935: __ movq(r11, rax);
Can you give r11 an alias, like saved_right?

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:967: ASSERT(op == Token::SHR);
It looks like all three four below are identical (including the cvtqsi2sd)
except for a little prologue. Please combine them, only adding the additional
prologue step if needed depending on mode_ and kSmiValueSize/op.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:1132: if (kSmiValueSize == 32) {
Remove the if and the "then" part of this code, just put the else version in
straight-line, which already asserts "kSmiValueSize == 31"

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4669: __ SmiAdd(rbx, rbx, rcx, &call_runtime);
I still think it's OK to use the overflow-checking version. This is because full
code gen is not super performance critical anyway, any critical code will be
Crankshafted. I would opt to use the version that is more maintainable and
readable, i.e. the  "__ SmiAdd(rbx, rbx, rcx, &call_runtime);" version.

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1402: if
(instr->hydrogen()->representation().IsSmi()) {
Is this really correct (here and below)? The non-smi case seems fine, but the
Smi case should be pushed into the Macro assembler, and shouldn't it call andl_
in the 31-bit smi case and and_ in the 32-bit case.

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1410: __ or_(ToRegister(left),
Immediate(right_operand));
Same here.

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1416: if
(instr->hydrogen()->representation().IsSmi()) {
Same here.

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1755: __ lea(ToRegister(instr->result()),
Here and below, I think all Smi operations should be in the MacroAssembler.

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

https://codereview.chromium.org/21014003/diff/34001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:378: class SmiFunctionInvoker {
I think approach works, but I think you can get away with a single virtual
method on the abstract super class (which should just be called ValidSmiChecker
or something like that)

virtual void DoCheckSmi(Register smi_candidate_reg);

Then you have two subclasses, LithiumValidSmiChecker that takes the LCodeGen and
environment in its constructor and implements it so:

virtual void DoCheckSmi(Register smi_candidate_reg) {
  if (kSmiSize == 31) {
    cmpl(src, Immediate(0xc0000000));
    code_gen()->DeoptimizeIf(zero, environment());
  }
 }

And stub generators/full code gen take a label in their constructor:

virtual void DoCheckSmi(Register smi_candidate_reg) {
  if (kSmiSize == 31) {
    cmpl(src, Immediate(0xc0000000));
    b(zero, fail_label());
  }
 }

}

[haitao.feng, 2013-08-12 09:54:24]

danno, thanks a lot for the review. All the comments are addressed. Please take
another look.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:904: if (result_type <= BinaryOpIC::INT32) {
Actually this code was taken from
https://chromiumcodereview.appspot.com/10937013 when we fixed the
deopt-minus-zero fail issue for X32. It is a little out-dated. Now I migrate the
latest IA32 part of code here to deal with 31-bit SMI. From debugging
deopt-minus-zero, I think this code could verify whether the result is  -0 also
as -0 could not be represented as int32.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:905: __ cvttsd2si(kScratchRegister, xmm0);
The test here is for Int32, instead of SMI.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:910: __ j(zero, non_int32_failure);
The main logic is at
https://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/ic.cc#2681.
For 32-bit SMI, the result type is heap number as int32 and SMI are the same for
32-bit SMI.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:935: __ movq(r11, rax);
On 2013/08/07 18:41:26, danno wrote:
> Can you give r11 an alias, like saved_right?

Done.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:967: ASSERT(op == Token::SHR);
On 2013/08/07 18:41:26, danno wrote:
> It looks like all three four below are identical (including the cvtqsi2sd)
> except for a little prologue. Please combine them, only adding the additional
> prologue step if needed depending on mode_ and kSmiValueSize/op.

Done.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:1132: if (kSmiValueSize == 32) {
On 2013/08/07 18:41:26, danno wrote:
> Remove the if and the "then" part of this code, just put the else version in
> straight-line, which already asserts "kSmiValueSize == 31"

Done.

https://codereview.chromium.org/21014003/diff/34001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:4669: __ SmiAdd(rbx, rbx, rcx, &call_runtime);
On 2013/08/07 18:41:26, danno wrote:
> I still think it's OK to use the overflow-checking version. This is because
full
> code gen is not super performance critical anyway, any critical code will be
> Crankshafted. I would opt to use the version that is more maintainable and
> readable, i.e. the  "__ SmiAdd(rbx, rbx, rcx, &call_runtime);" version.

Done.

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
File src/x64/lithium-codegen-x64.cc (right):

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1402: if
(instr->hydrogen()->representation().IsSmi()) {
It is really correct. This code utilizes a hardware feature that immediate32
will be signed extend to 64-bit. The full bitwise operation (and_, or_ and xor_)
works for both 32-bit SMI and 31-bit SMI. For 31-bit SMI, from 63rd to 30th bit
are all the same for an operand,  "bitop_(src1, src2)" has the same result of
"bitl(src1, src2); movsxlq(src1, src1);".

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1410: __ or_(ToRegister(left),
Immediate(right_operand));
Explained above.

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1416: if
(instr->hydrogen()->representation().IsSmi()) {
Explained above.

https://codereview.chromium.org/21014003/diff/34001/src/x64/lithium-codegen-x...
src/x64/lithium-codegen-x64.cc:1755: __ lea(ToRegister(instr->result()),
On 2013/08/07 18:41:26, danno wrote:
> Here and below, I think all Smi operations should be in the MacroAssembler.

Done.

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

https://codereview.chromium.org/21014003/diff/34001/src/x64/macro-assembler-x...
src/x64/macro-assembler-x64.h:378: class SmiFunctionInvoker {
Thanks for the recommendation. I used a slightly different version:

  class SmiInstructionWrapper {                                                 
   public:                                                                      
    SmiInstructionWrapper() { }                                                 
    virtual ~SmiInstructionWrapper() { }                                        
    virtual bool NeedsCheckOverflow() const = 0;                                
    virtual bool NeedsCheckMinusZero() const = 0;                               
    virtual bool NeedsKeepSourceOperandsIntact() const = 0;                     
    virtual void BailoutIf(Condition cc) const = 0;                             
  };                                                                            

It seems that BailoutIf(Condition cc) is more generally. I have used it to
handle SmiShiftLeftConstant, SmiAdd and SmiSub. It should be able to handle
SmiMul (a work in progress at https://codereview.chromium.org/22600005/).

[danno, 2013-08-19 21:47:44]

Thank you for the next patch, sorry it took so long to respond to it, but it's
quite a tricky problem how to design a clear API for supporting both 31- and 32-
bit smis and still keeping a clear, simple division of responsibilities.

In my review, I focused on your changes to the macro assembler and the wrapper
classes (which I think should be renamed to have a super class called
SmiOperationOverflowHandler). The changes I reviewed are going in the right
direction but I suspect will still need several iterations.

I encourage you to break out the unrelated changed, as you already have for
some, to make landing those easier.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/full-codeg...
File src/x64/full-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/full-codeg...
src/x64/full-codegen-x64.cc:4454: __ SmiAddConstant(rax, rax, Smi::FromInt(1));
Shouldn't overflow also be handled in SmiAddConstant by passing in a wrapper?

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.cc (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:990: static inline Immediate SmiToImmediate(Smi*
src) {
Why not add a constructor to the Immeidate constructor in the x64 assembler that
takes a Smi as a parameter? It can ASSERT that it only called if
SmiValuesAre31Bits() is true (no if) and then return the properly initialized
Immediate value directly. That approach makes this helper function unnecessary.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:1705: if
(wrapper.NeedsKeepSourceOperandsIntact()) {
Here and everywhere else for the Add/Sub operations. Why not define the
interface as taking a separate parameter (not the wrapper) that specifies if the
call site cares about clobbering src1 if it's dst on the overflow case. The code
can then use the trick that you've seen in the stub in your other CL:

addq(src1, src2);
j(no_overflow, local_done);
subq(src1, src2);
wrapper.BailoutIf(always);
bind(local_done);

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:1955: const SmiInstructionWrapper& wrapper) {
This code seems identical to the version directly above. Can you templatize the
core and just have two wrappers that call either with a Register or Operand?

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:1966: if (wrapper.NeedsCheckOverflow())
wrapper.BailoutIf(overflow);
Always call BaikoutIf, here and elsewhere.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:2067: const SmiInstructionWrapper& wrapper) {
This code seems identical to the version directly above. Can you templatize the
core and just have two wrappers that call either with a Register or Operand?

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:2497: ASSERT(!dst.is(rcx));
Here and elsewhere, I think it really makes testability more difficult if the
register constraints are different between 32- and 31-bit smis. For now, you can
ASSERT(!dst.is(src1)) in all cases, that should work for full-code-gen and stub
usages. 

Alternatively (maybe better from an abstraction point of view, but probably
outside the scope of this CL), you can always pass an extra scratch register
(like r9) to SmiShiftLogicalRight and the other routines so that dst can
actually be src1 and things still work (that removes the need for the special
handling of the dst register at the caller site that you added).

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.h (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:378: class SmiInstructionWrapper {
My general feedback to this class is that it's too complex. You seem to be
mixing responsibilities. e.g. whether a particular SMI operation needs to check
for overflow is a decision of the calling site of the macro instruction, less so
a policy of whether you're generating full code gen code or not. Same for
NeedsCheckMinusZero, it seems like something that is passed in separately. 

How about this instead (including a rename thatI think is important):

class SmiOperationOverflowHandler {
 public:
  virtual void BailoutIf(Condition cc) const = 0;
}

// Use this for callers that don't care about overflow at all.
class IgnoreSmiOverflowHandler : public SmiOperationOverflowHandler{
  virtual void BailoutIf(Condition cc) const {}
}

class JumpOnSmiOverflowHandler :  public SmiOperationOverflowHandler {
  virtual void BailoutIf(Condition cc) const { DeoptimizeIf(cc, env) }
}

class DeoptOnSmiOverflowHandler : public SmiOperationOverflowHandler {
  SmiOperationOverflowHandler(bool can_overflow) {...}
  virtual void BailoutIf(Condition cc) const { if (can_overflow_) {
DeoptimizeIf(cc, env); } }
}

NeedsKeepSourceOperandsIntact also doesn't seem to belong here. It seems that
there are two different types of Smi operations. Some, like Add and Sub, have
some callers that want to make sure that their input register are preserved in
certain situations... this is a per-call-site decision. Others, like
SmiShiftLeft, didn't care if dst is the same as src1/src2, clobbering was OK.
That should still remain the case when you add 31-bit smi support, which might
mean that you need to pass in an addition temp register so that you don't have
to write different or convoluted code at the caller site in the 31/32 bit smi
cases.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:383: virtual bool NeedsCheckMinusZero() const = 0;
You never call NeedsCheckMinusZero anywhere. Can you remove it.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:384: virtual bool NeedsKeepSourceOperandsIntact()
const = 0;
I think you can avoid this one, too. See my comments in the .cc file.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:388: class SafeSmiInstructionWrapper : public
SmiInstructionWrapper {
This class is never used, please remove it.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:428: bool IsUnsafeInt(const int x);
shouldn't this be a int32_t just to be clear?

[haitao.feng, 2013-08-20 15:09:30]

danno, thanks a lot for the review. I appreciate it. I know you are quite busy
preparing Chrome 30.

I will think about the best approach to separate policy and mechanism to address
the problem that SMI macro instructions could be called by Lithium and
FullCodeGen/Stubs and make 31-bit SMI incrementally reviewed and committed.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/full-codeg...
File src/x64/full-codegen-x64.cc (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/full-codeg...
src/x64/full-codegen-x64.cc:4454: __ SmiAddConstant(rax, rax, Smi::FromInt(1));
I will address this in https://codereview.chromium.org/22935005/.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.cc (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:990: static inline Immediate SmiToImmediate(Smi*
src) {
This is what I have done originally at
https://codereview.chromium.org/21014003/patch/1/1005.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:1705: if
(wrapper.NeedsKeepSourceOperandsIntact()) {
Thanks for the recommendation. I will use that trick to avoid the
kScratchRegister usage.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:1955: const SmiInstructionWrapper& wrapper) {
I will do that.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.h (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:383: virtual bool NeedsCheckMinusZero() const = 0;
I have planned to use it for SmiMul. If the caller is in lithium, we could avoid
check for minus zero if HValue::kBailoutOnMinusZero flag is false.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:388: class SafeSmiInstructionWrapper : public
SmiInstructionWrapper {
I have planned to unify the interface. So the current SMI functions which do not
check overflow/minusZero could use a singleton SafeSmiInstrucitonWrapper.

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:428: bool IsUnsafeInt(const int x);
I will change this when I gather all the SMI functions together.

[danno, 2013-08-20 15:58:14]

Hi Haitao,

Thanks for your continued iteration on this CL. I know it's a lot of work, and I
appreciate you persistence and patience working through some of these trick
issues. I really want to make sure that we get this right, even if it takes
longer, since it will be the foundation of a lot of work in the future. It's
definitely heading in the right direction!

Regards,
Danno

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.cc (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:990: static inline Immediate SmiToImmediate(Smi*
src) {
On 2013/08/20 15:09:30, haitao.feng wrote:
> This is what I have done originally at
> https://codereview.chromium.org/21014003/patch/1/1005. 

Ooops! I'm sorry, I apologize, that's embarrassing. 

The old version from 21014003 bothered me a bit because it was #ifdefed, I think
without the #ifdef, I would be less adverse to the old version. This new version
has an "if", which also doesn't seem quite right. So, adding to the assembler or
here is fine, I'll leave it up to you, but it should be without an #ifdef or if,
e.g.

constructor:

explicit Immediate(Smi* value) {
  ASSERT(SmiValuesAre31Bits());
  value_ = static_cast<int32_t>(reinterpret_cast<intptr_t>(value));
}

helper function:

static inline Immediate SmiToImmediate(Smi* src) {
  ASSERT(SmiValuesAre31Bits());
  return Immediate(static_cast<int32_t>(reinterpret_cast<intptr_t>(src)));
}

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.h (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:383: virtual bool NeedsCheckMinusZero() const = 0;
On 2013/08/20 15:09:30, haitao.feng wrote:
> I have planned to use it for SmiMul. If the caller is in lithium, we could
avoid
> check for minus zero if HValue::kBailoutOnMinusZero flag is false.
> 

Yes, but this is specific to the Mul operation, seems like it could just be a
"MinusZeroHandlingMode" parameter to the SmiMul/SmiDiv macro assembler
instruction?

[haitao.feng, 2013-08-21 13:18:03]

Hi Danno,

Thanks a lot for your comments, recommendations and guidance. I am glad that the
direction of this CL is clear after you reviewed all the codes in this CL
(including the macro assembler instruction definition and usage). I will work
incrementally to get it reviewed/committed as you have recommended.

Thanks
-Haitao

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.cc (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.cc:990: static inline Immediate SmiToImmediate(Smi*
src) {
I will use the constructor with an assertion in the future CL. A comment on the
"if" statement: it could be optimized away by gcc at static time, so there is no
additional runtime overhead for "SmiValuesAre32Bits" and "SmiValuesAre31Bits".

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
File src/x64/macro-assembler-x64.h (right):

https://chromiumcodereview.appspot.com/21014003/diff/45001/src/x64/macro-asse...
src/x64/macro-assembler-x64.h:383: virtual bool NeedsCheckMinusZero() const = 0;
We will handle this when https://codereview.chromium.org/22600005/ is landed.
Currently the difference between Lithium and FullCodeGen/Stub when calling SMI
macro instruction is:
  1) dst and src1 are same from Lithium, and we do not need to keep src1 intact.
  2) overflow check could be omitted.
  3) minusZero check could be omitted. 

I will think about your recommendations to abstract those difference.