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#newcode3032
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#newcode382
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#newcode996
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#newcode1393
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#newcode1099
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#newcode125
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?
https://codereview.chromium.org/21014003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
