Refactor Math.min/max to be a single HInstruction.

Refactor Math.min/max to be a single HInstruction.

That allows us to dynamically compute representations and insert appropriate HChange instructions.

Committed: https://code.google.com/p/v8/source/detail?r=12265

[Jakob Kummerow, 2012-08-03 16:13:14]

Please review.

Erik: Please look at the ARM specific changes.

Yang: Please look at everything else (ia32/x64/hydrogen).

[Erik Corry, 2012-08-06 10:44:39]

ARM part LGTM

http://codereview.chromium.org/10829169/diff/1/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/10829169/diff/1/src/arm/disasm-arm.cc#newcode1222
src/arm/disasm-arm.cc:1222: instr->Opc1Value() == 0x2) {
This tests bits 20, 21, 23, 24, but ignores bits 25-27 and 22 and 4-7.  Should
it not say 'Unknown' if they do not match the expectations for vorr?

http://codereview.chromium.org/10829169/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/10829169/diff/1/src/arm/lithium-codegen-arm.cc...
src/arm/lithium-codegen-arm.cc:1687: __ vadd(result_reg, left_reg, right_reg);
Should this be vand?

http://codereview.chromium.org/10829169/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

http://codereview.chromium.org/10829169/diff/1/src/arm/simulator-arm.cc#newco...
src/arm/simulator-arm.cc:2679: instr->Opc1Value() == 0x2) {
Same question here as in the disassembler?

[Yang, 2012-08-06 10:50:41]

LGTM with comments.

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/lithium-codege...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:1687: __ vadd(result_reg, left_reg, right_reg);
On 2012/08/06 10:44:40, Erik Corry wrote:
> Should this be vand?

I think vadd would work equally well here:
-0 + -0 = -0
-0 + 0 = 0
0 + -0 = 0
0 + 0 = 0

https://chromiumcodereview.appspot.com/10829169/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:2559: // the right side.
Please update comment.

https://chromiumcodereview.appspot.com/10829169/diff/1/src/ia32/lithium-codeg...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:1532: __ j(equal, &check_zero, Label::kNear); 
// left == right.
As discussed offline, it might be worth branching on zero-flag first and only in
case the zero-flag is set, check for parity flag. For the usual case where
arguments are not equal, we can save one jump instruction.

[Erik Corry, 2012-08-06 11:28:39]

An extra afterthought: There are test-assembler-arm.cc and
test-disassembler-arm.cc files and they should have vorr added to them.

[Jakob Kummerow, 2012-08-06 14:08:43]

Addressed feedback.

Also removed support for VORR because it is a NEON instruction.

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/disasm-arm.cc#...
src/arm/disasm-arm.cc:1222: instr->Opc1Value() == 0x2) {
On 2012/08/06 10:44:40, Erik Corry wrote:
> This tests bits 20, 21, 23, 24, but ignores bits 25-27 and 22 and 4-7.  Should
> it not say 'Unknown' if they do not match the expectations for vorr?

As discussed, removed vorr completely.

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/lithium-codege...
File src/arm/lithium-codegen-arm.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/lithium-codege...
src/arm/lithium-codegen-arm.cc:1687: __ vadd(result_reg, left_reg, right_reg);
On 2012/08/06 10:50:41, Yang wrote:
> On 2012/08/06 10:44:40, Erik Corry wrote:
> > Should this be vand?
> 
> I think vadd would work equally well here:
> -0 + -0 = -0
> -0 + 0 = 0
> 0 + -0 = 0
> 0 + 0 = 0

Added a comment explaining why "vadd" is not a typo.

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/arm/simulator-arm....
src/arm/simulator-arm.cc:2679: instr->Opc1Value() == 0x2) {
On 2012/08/06 10:44:40, Erik Corry wrote:
> Same question here as in the disassembler?

Same answer: removed vorr.

https://chromiumcodereview.appspot.com/10829169/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:2559: // the right side.
On 2012/08/06 10:50:41, Yang wrote:
> Please update comment.

Done.

https://chromiumcodereview.appspot.com/10829169/diff/1/src/ia32/lithium-codeg...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/10829169/diff/1/src/ia32/lithium-codeg...
src/ia32/lithium-codegen-ia32.cc:1532: __ j(equal, &check_zero, Label::kNear); 
// left == right.
On 2012/08/06 10:50:41, Yang wrote:
> As discussed offline, it might be worth branching on zero-flag first and only
in
> case the zero-flag is set, check for parity flag. For the usual case where
> arguments are not equal, we can save one jump instruction. 

As discussed offline, I'd prefer to keep the architectures in sync. Hopefully
the branch prediction is good enough to mitigate the difference.