Bug 23070 – Allow JSImmediate to store a full 32-bit int on 64-bit platforms.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

Presently the top 32-bits of a 64-bit JSImmediate serve as a sign extension of a 31-bit int stored in the low word (shifted left by one, to make room for a tag).  In the new format, the top 31-bits serve as a sign extension of a 32-bit int, still shifted left by one.

[Gavin Barraclough]

Created attachment 26355 [details]
The patch

[Darin Adler]

Comment on attachment 26355 [details]
The patch

> +        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

[Gavin Barraclough]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/assembler/MacroAssembler.h
Sending        JavaScriptCore/assembler/X86Assembler.h
Sending        JavaScriptCore/jit/JIT.cpp
Sending        JavaScriptCore/jit/JIT.h
Sending        JavaScriptCore/jit/JITArithmetic.cpp
Sending        JavaScriptCore/jit/JITInlineMethods.h
Sending        JavaScriptCore/runtime/JSImmediate.h
Sending        JavaScriptCore/wtf/Platform.h
Transmitting file data .........
Committed revision 39540.