Bug 23070 - Allow JSImmediate to store a full 32-bit int on 64-bit platforms.
Summary: Allow JSImmediate to store a full 32-bit int on 64-bit platforms.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-01 17:59 PST by Gavin Barraclough
Modified: 2009-01-01 21:40 PST (History)
0 users

See Also:


Attachments
The patch (83.38 KB, patch)
2009-01-01 17:59 PST, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2009-01-01 17:59:20 PST
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.
Comment 1 Gavin Barraclough 2009-01-01 17:59:40 PST
Created attachment 26355 [details]
The patch
Comment 2 Darin Adler 2009-01-01 18:14:25 PST
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
Comment 3 Gavin Barraclough 2009-01-01 21:40:25 PST
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.