[webkit-reviews] review granted: [Bug 23070] Allow JSImmediate to store a full 32-bit int on 64-bit platforms. : [Attachment 26355] The patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 23070: Allow JSImmediate to store a full 32-bit int on 64-bit platforms.
https://bugs.webkit.org/show_bug.cgi?id=23070

Attachment 26355: The patch
https://bugs.webkit.org/attachment.cgi?id=26355&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   The new benhavior is enabled using a flag in Platform.h,
'WTF_USE_ALTERNATE_JSIMMEDIATE'.

Typo, "benhavior".

> +	   When this is set the contatants defining the range of ints allowed
to be stored as

Typo, "contatants".

> +	   format.  This patch updates tehe JIT so that it can also operate
with the new format.

Typo, "tehe".

> -#define CAN_SIGN_EXTEND_8_32(value) (value == ((int)(signed char)value))
> +#define CAN_SIGN_EXTEND_8_32(value) ((int32_t)value == ((int32_t)(signed
char)value))
> +#if PLATFORM(X86_64)
> +#define CAN_SIGN_EXTEND_32_64(value) ((intptr_t)value ==
((intptr_t)(int32_t)value))
> +#define CAN_SIGN_EXTEND_U32_64(value) ((intptr_t)value ==
((intptr_t)(uint32_t)value))
> +#endif

Can these be inline functions rather than macros? I don't see any benefit to
using a macro for this kind of test.

> +	   void compileFastArith_op_add(Instruction* currentInstruction);
> +	   void compileFastArith_op_mul(Instruction* currentInstruction);

I think we could omit the argument name "currentInstruction" with no loss of
clarity, and so we should.

> +	   void compileFastArithSlow_op_add(Instruction* currentInstruction,
Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_mul(Instruction* currentInstruction,
Vector<SlowCaseEntry>::iterator& iter);

Ditto. Also we should definitely leave out the name "iter", which adds nothing.


> +	   void compileFastArithSlow_op_mod(unsigned result, unsigned op1,
unsigned op2, Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_bitand(unsigned result, unsigned op1,
unsigned op2, Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_lshift(unsigned result, unsigned op1,
unsigned op2, Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_rshift(unsigned result, unsigned op1,
unsigned op2, Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_pre_inc(unsigned srcDst,
Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_pre_dec(unsigned srcDst,
Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_post_inc(unsigned result, unsigned
srcDst, Vector<SlowCaseEntry>::iterator& iter);
> +	   void compileFastArithSlow_op_post_dec(unsigned result, unsigned
srcDst, Vector<SlowCaseEntry>::iterator& iter);

Please leave out the "iter" here.

> +#if USE(ALTERNATE_JSIMMEDIATE)
> +#else
> +#endif

Pleas remove this.

> +
> +#if USE(ALTERNATE_JSIMMEDIATE)
> +#else
> +#endif
> +
> +
> +
> +
> +
>  #if !ENABLE(JIT_OPTIMIZE_ARITHMETIC)

Please remove this.

> -static ALWAYS_INLINE uintptr_t asInteger(JSValue* value)
> -{
> -    return reinterpret_cast<uintptr_t>(value);
> -}

I like to use these sorts of inline functions to add type safety to the many
reinterpret_cast that are needed in the code. I notice that the new code does
the casts directly. You should consider the inline function approach.

r=me