Optimize bitops with non-Smi inputs. Instead of converting both inputs...

Optimize bitops with non-Smi inputs.  Instead of converting both inputs
to floating point and then converting back we convert directly to a
32 bit integer.  In addition the bit twiddling implementation of float-
to-integer conversion has been ported from ARM.  Testing has shown that
this runs faster than the x87 or SSE3 rounding instructions.  This change
is IA32 only.  There may be a smaller benefit from doing the same on x64.

Committed: http://code.google.com/p/v8/source/detail?r=3492

[Lasse Reichstein, 2009-12-17 16:35:50]

Not done. Looks good so far

http://codereview.chromium.org/506052/diff/1/2
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/506052/diff/1/2#newcode7172
src/ia32/codegen-ia32.cc:7172: __ mov(ecx, Immediate(0));
Use xor to zero.

http://codereview.chromium.org/506052/diff/1/2#newcode7173
src/ia32/codegen-ia32.cc:7173: // Check whether the exponent matches a 32 bit
signed int that is not a Smi.
"is not a Smi" -> "cannot be represented as a smi".
We have non-smis that could be smis.

http://codereview.chromium.org/506052/diff/1/2#newcode7174
src/ia32/codegen-ia32.cc:7174: // A non-Smi integer is 1.xxx * 2^30 so the
exponent is 30 (biased).  This is
"A non-smi 32-bit integer is ..."

http://codereview.chromium.org/506052/diff/1/2#newcode7192
src/ia32/codegen-ia32.cc:7192: // We have the big exponent from >>>.
... or from some other operation that gives a result in the range 2^31..2^32-1.

http://codereview.chromium.org/506052/diff/1/2#newcode7201
src/ia32/codegen-ia32.cc:7201: // distance.
Change last sentence to (something like):
We have kExponentShift+1 significant bits in the low end of the word. Shift them
to to the top bits.

http://codereview.chromium.org/506052/diff/1/2#newcode7206
src/ia32/codegen-ia32.cc:7206: // Shift down 21 bits to get the last 11 bits.
Explain, or replace, the constants:
  21 = big_shift_distance
  11 = 32 - big_shift_distance

http://codereview.chromium.org/506052/diff/1/2#newcode7213
src/ia32/codegen-ia32.cc:7213: __ neg(ecx);
Slightly shorter:
test(scratch, scratch)
j(positive, &done)
neg(ecx)

http://codereview.chromium.org/506052/diff/1/2#newcode7216
src/ia32/codegen-ia32.cc:7216: // End of handling for big exponent.
You could wrap the entire big-exponent section in a statement block. It gives
scope to the variables and groups and indents the code for easier reading.

http://codereview.chromium.org/506052/diff/1/2#newcode7224
src/ia32/codegen-ia32.cc:7224: (HeapNumber::kExponentBias + 0) <<
HeapNumber::kExponentShift;
I think (0 + HeapNumber::kExponentBias) is more readable (but that's obviuously
a matter of taste).

http://codereview.chromium.org/506052/diff/1/2#newcode7232
src/ia32/codegen-ia32.cc:7232: __ sub(ecx, Operand(scratch2));
Complex rewrite: If you make ecx one larger here (loading immediate 31), and
then below increase shift_distance by one as well to shift out the sign bit out,
then the shr_cl further down will shift in a zero sign bit. This allows you to
drop the and_ with kMantissaMask.
(I think.)

http://codereview.chromium.org/506052/diff/1/2#newcode7242
src/ia32/codegen-ia32.cc:7242: // we want to leave the sign bit 0 so we subtract
2 bits from the shift
"leave the sign bit 0" -> "leave a zero sign bit"

http://codereview.chromium.org/506052/diff/1/2#newcode7248
src/ia32/codegen-ia32.cc:7248: // it's probably slower to test than just to do
it.
True. The load is probably from the first level cache.

http://codereview.chromium.org/506052/diff/1/2#newcode7250
src/ia32/codegen-ia32.cc:7250: // Shift down 22 bits to get the last 10 bits.
Explain or replace constants.

http://codereview.chromium.org/506052/diff/1/2#newcode7267
src/ia32/codegen-ia32.cc:7267: 
Only reviewed until here.

[Lasse Reichstein, 2009-12-18 08:06:45]

LGTM

http://codereview.chromium.org/506052/diff/1/2
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/506052/diff/1/2#newcode7267
src/ia32/codegen-ia32.cc:7267: 
But there wasn't much more, I can see now.

[sra1, 2009-12-18 08:30:21]

I do find that the bit-twiddling is faster to be surprising.
How much of the win is from avoiding a useless SMI->FP->int32 conversion vs.
avoiding the FISTTP instruction?

A branch mispredict is several times slower than a FP conversion, so be careful
that your benchmarks are realistic.
It might also be worth running the code under VTune to see if the old code was
stalling due to an unfortunate microarchitectural issue that could be fixed with
a little instruction scheduling.
I can imagine that fnstsw, having a value dependency on the exception state,
might be delayed for the latency of the previous instruction(s).


I see some benchmarks have code like (x & 0xFFFFFFFF) or the same but with
dynamic values.  It would be a huge advantage in this case if the FP->int
conversion took care of ToInt32 / ToUint32 conversions specified for the bitops.
 
I was considering investigating if the low 32 bits of FISTTP mem64 would always
be the right value.  I think it would for values with magnitude < 2^51, but I
was not sure outside that range.

[Erik Corry, 2009-12-18 09:33:36]

Everything else fixed.

http://codereview.chromium.org/506052/diff/1/2
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/506052/diff/1/2#newcode7232
src/ia32/codegen-ia32.cc:7232: __ sub(ecx, Operand(scratch2));
On 2009/12/17 16:35:50, Lasse Reichstein wrote:
> Complex rewrite: If you make ecx one larger here (loading immediate 31), and
> then below increase shift_distance by one as well to shift out the sign bit
out,
> then the shr_cl further down will shift in a zero sign bit. This allows you to
> drop the and_ with kMantissaMask.
> (I think.)

Nice idea, but it ruins the fast case for zero shift or zero answer, so I don't
think it's worth it.