ARM: Fast path for compare against constant.

ARM: Fast path for compare against constant.

BUG=none
TEST=none

[Mads Ager (chromium), 2011-07-27 13:05:05]

Redirecting because of vacation. Bill, can you have a look?

Is any of this actually ARM specific? It doesn't look that way. If it is not,
please port to the other architectures as well.

http://codereview.chromium.org/7489045/diff/1/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/7489045/diff/1/src/arm/constants-arm.h#newcode179
src/arm/constants-arm.h:179: inline bool EvalComparison(Condition cond, double
op1, double op2) {
Why not have this in the code generator and switch on the token instead of going
from token to condition to operation?

[Alexandre, 2011-07-28 13:42:15]

I ported the code to ia32 and x64.
Note that I could not perform any tests for x64 as I do not have a 64-bit
machine handy.

Can you have a look at the use of LCodeGen::TokenToCondition in
LCodeGen::DoCmpIDAndBranch?
I fixed the use of 'instr->is_double()' for the 'is_unsigned' parameter on ARM.
I do not know much about ia32 or x64 comparisons, but this looks dubious to me.

Also, the comparison of two constants might be done at a higher level in
hydrogen. The compilation of unreachable blocks could even be avoided.

Thanks,

Alexandre

http://codereview.chromium.org/7489045/diff/1/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/7489045/diff/1/src/arm/constants-arm.h#newcode179
src/arm/constants-arm.h:179: inline bool EvalComparison(Condition cond, double
op1, double op2) {
On 2011/07/27 13:05:05, Mads Ager wrote:
> Why not have this in the code generator and switch on the token instead of
going
> from token to condition to operation?

Done.

[William Hesse, 2011-08-11 09:56:24]

Is this issue still live?  Shall I review it?

[William Hesse, 2011-08-11 12:08:22]

Token::EvalComparison should be moved to src/assembler.cc.  That is a better
place for platform-independent operations on constants, used when generating
code.

Other than that, it looks good to me.  (LGTM.)

[William Hesse, 2011-09-09 08:53:19]

LGTM.  I didn't realize you had made the change - I was waiting for a message
from you.  I will commit.

[William Hesse, 2011-09-09 11:14:46]

Needs to be fixed, as described in comments.
Please include a message when uploading, so I get an email.

http://codereview.chromium.org/7489045/diff/16001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/7489045/diff/16001/src/assembler.cc#newcode1115
src/assembler.cc:1115: ASSERT(IsCompareOp(op));
Needs to be Token::IsCompareOp(op).

http://codereview.chromium.org/7489045/diff/16001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/7489045/diff/16001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:1556: __ cmpl(ToOperand(left), Immediate(value));
This case is not needed, because left is created with "UseRegisterAtStart".
None of the new code is ever hit, because of how left and right are created in
lithium-x64.cc.
This needs to be changed - I would recommend
UseRegisterOrConstantAtStart for integer left
UseOrConstantAtStart for integer right

UseRegisterOrConstantAtStart for double left
UseRegisterOrConstantAtStart for double right.

Then the only one that could be a memory operand is right.

These changes should also be made to the ia32 platform.

http://codereview.chromium.org/7489045/diff/16001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:1568: __ cmp(ToRegister(left), ToOperand(right));
This must be a cmpl.

[Alexandre, 2011-09-09 17:07:22]

Thanks for the comments. The new patch will be uploaded on Monday.

I tested it on ia32 and it works fine.
There might be a better way to handle the double case, but I don't how how to
handle that for ia32/x64.

Again I could not test the x64 code.
If minor changes are needed, could you please do it?

Thanks!

Alexandre

http://codereview.chromium.org/7489045/diff/16001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/7489045/diff/16001/src/assembler.cc#newcode1115
src/assembler.cc:1115: ASSERT(IsCompareOp(op));
On 2011/09/09 11:14:46, William Hesse wrote:
> Needs to be Token::IsCompareOp(op).

Done.

http://codereview.chromium.org/7489045/diff/16001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/7489045/diff/16001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:1556: __ cmpl(ToOperand(left), Immediate(value));
On 2011/09/09 11:14:46, William Hesse wrote:
> This case is not needed, because left is created with "UseRegisterAtStart".
> None of the new code is ever hit, because of how left and right are created in
> lithium-x64.cc.
> This needs to be changed - I would recommend
> UseRegisterOrConstantAtStart for integer left
> UseOrConstantAtStart for integer right
> 
> UseRegisterOrConstantAtStart for double left
> UseRegisterOrConstantAtStart for double right.
> 
> Then the only one that could be a memory operand is right.
> 
> These changes should also be made to the ia32 platform.

I cleaned up this case. I hope ToRegister will work here.

Changed the integer register allocation in lithium.
For the double I added code to check that the two values are constant. Else we
use registers. Otherwise one double reg + one double constant fails.
I did the same on x64.

http://codereview.chromium.org/7489045/diff/16001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:1568: __ cmp(ToRegister(left), ToOperand(right));
On 2011/09/09 11:14:46, William Hesse wrote:
> This must be a cmpl.

Done on x64.

[William Hesse, 2011-10-20 10:28:40]

LGTM.

Committed as r9718, using code review http://codereview.chromium.org/8352040/.

Sorry it took so long.

http://codereview.chromium.org/7489045/diff/27001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7489045/diff/27001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1559: __ cmp(ToOperand(left),
ToImmediate(right));
If the immediate value is 0, this can also be test(left, left) on ia32 and x64. 
You should try this on ARM, if possible.  I'll test it on ia32 and x64.  Also,
this can be a ToRegister(left)  (fixed).

http://codereview.chromium.org/7489045/diff/27001/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):

http://codereview.chromium.org/7489045/diff/27001/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.cc:1565: __ cmpl(ToRegister(left),
ToOperand(right));
The register and memory operand cases for right must be handled separately. 
ToOperand does not work for registers on x64 platform.  Fixed.

[Alexandre, 2011-10-20 10:59:49]

For your information, just a comment about cmp vs tst on ARM.

Alexandre

http://codereview.chromium.org/7489045/diff/27001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7489045/diff/27001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:1559: __ cmp(ToOperand(left),
ToImmediate(right));
I forgot to change it to tst for Intel.
I intentionally used a cmp as it is better on ARM:
- the immediate is encoded in the instruction.
- cmp sets all the flags, whereas tst doesn't, resulting in more complex
handling for the pipeline.

On 2011/10/20 10:28:40, William Hesse wrote:
> If the immediate value is 0, this can also be test(left, left) on ia32 and
x64. 
> You should try this on ARM, if possible.  I'll test it on ia32 and x64.  Also,
> this can be a ToRegister(left)  (fixed).