ARM: Avoid VMSR instruction when converting to clamped uint8

ARM: Avoid VMSR instruction when converting to clamped uint8

Setting the FPSCR flags is expensive on some CPUs. Instead, do rounding
to the nearest integer by adding 0.5 and truncating towards zero.
Additional checks are needed to make sure that numbers ending in exactly
.5 get rounded to the nearest even number according to WebIDL spec, but
this case is relatively rare and even then the benefits outweigh the
costs. Performance increases ranging from 2x to 9x were seen in
measurements with a hot-loop benchmark depending on the CPU architecture.

This new code path is used on VFP3-capable processors, which have the
fixed-point conversion functions.

Generic ARM assembler, disassembler, and simulator functions are added
for the conversion between fixed point and floating point. A bug in
encoding the immediate value in the vcvt_f64_s32 instruction is also
fixed by this change. This did not cause behavioral issues before, since
in existing uses of the function the order of the bits in the immediate
value does not matter, as they are all 1.

BUG=3253
LOG=N

[jbramley, 2014-04-03 09:22:48]

https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (left):

https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3819: // Set rounding mode to round to the
nearest integer by clearing bits[23:22].
Shouldn't we be in the right rounding mode already? ECMAScript maths operations
are supposed to be done in round-to-nearest mode, and this is also the default
FPSCR setting. If we've changed it explicitly somewhere else (for whatever
reason), we're probably not doing normal ECMAScript maths properly.

If I'm correct about that, the whole thing collapses down:

// Handle inputs >= 255 (including +infinity).
mov(result_reg, 255);
Vmov(double_scratch, 255.0, result_reg);
VFPCompareAndSetFlags(input_reg, double_scratch);
b(ge, &done);

// All other inputs will clamp to the range [0-255]: NaN and -infinity both
produce 0.
vcvt_u32_f64(double_scratch.low(), input_reg, kFPSCRRounding);
Vmov(result_reg, double_scratch.low());

This is more-or-less equivalent to what we did in ClampDoubleToUint8 in
src/arm64/macro-assembler-arm64.cc, though the available instructions make it
much simpler in A64.

https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3800: b(le, &done);  // Double value is <= 0, NaN
or Inf, return 0.
Actually there are only two input ranges where vcvt.u32 doesn't agree with this
clamp operation:

 - Values greater than 255 (since vcvt only converts to (u)int32 values.
 - +infinity, where vcvt produces 0 but we want 255.

We can catch both of these ranges with the >=255 test, so the <=0 test can be
omitted entirely.

(The old implementation used vcvt.s32 so the zero check was necessary there.)

[oetuaho-nv, 2014-04-03 16:02:45]

Thanks for the quick review, one comment inline.

I can look into this alternative solution and submit a new version once I have
code and test results ready, and submit the assembler bug fix separately.

https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (left):

https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm....
src/arm/macro-assembler-arm.cc:3819: // Set rounding mode to round to the
nearest integer by clearing bits[23:22].
On 2014/04/03 09:22:48, jbramley wrote:
> Shouldn't we be in the right rounding mode already? ECMAScript maths
operations
> are supposed to be done in round-to-nearest mode, and this is also the default
> FPSCR setting. If we've changed it explicitly somewhere else (for whatever
> reason), we're probably not doing normal ECMAScript maths properly.
> 
> If I'm correct about that, the whole thing collapses down:
> 
> // Handle inputs >= 255 (including +infinity).
> mov(result_reg, 255);
> Vmov(double_scratch, 255.0, result_reg);
> VFPCompareAndSetFlags(input_reg, double_scratch);
> b(ge, &done);
> 
> // All other inputs will clamp to the range [0-255]: NaN and -infinity both
> produce 0.
> vcvt_u32_f64(double_scratch.low(), input_reg, kFPSCRRounding);
> Vmov(result_reg, double_scratch.low());
> 
> This is more-or-less equivalent to what we did in ClampDoubleToUint8 in
> src/arm64/macro-assembler-arm64.cc, though the available instructions make it
> much simpler in A64.

This would be an excellent solution, but it seems the FPSCR state can be wrong
when entering this function. VFPEnsureFPSCRState doesn't currently set the
rounding mode, and I suppose it can be messed with by outside code. But if
setting the FPSCR state in VFPEnsureFPSCRState will be enough, this solution
could work.

[Rodolph Perfetta, 2014-04-03 18:29:20]

On 2014/04/03 16:02:45, oetuaho-nv wrote:
> Thanks for the quick review, one comment inline.
> 
> I can look into this alternative solution and submit a new version once I have
> code and test results ready, and submit the assembler bug fix separately.
> 
>
https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm.cc
> File src/arm/macro-assembler-arm.cc (left):
> 
>
https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm....
> src/arm/macro-assembler-arm.cc:3819: // Set rounding mode to round to the
> nearest integer by clearing bits[23:22].
> On 2014/04/03 09:22:48, jbramley wrote:
> > Shouldn't we be in the right rounding mode already? ECMAScript maths
> operations
> > are supposed to be done in round-to-nearest mode, and this is also the
default
> > FPSCR setting. If we've changed it explicitly somewhere else (for whatever
> > reason), we're probably not doing normal ECMAScript maths properly.
> > 
> > If I'm correct about that, the whole thing collapses down:
> > 
> > // Handle inputs >= 255 (including +infinity).
> > mov(result_reg, 255);
> > Vmov(double_scratch, 255.0, result_reg);
> > VFPCompareAndSetFlags(input_reg, double_scratch);
> > b(ge, &done);
> > 
> > // All other inputs will clamp to the range [0-255]: NaN and -infinity both
> > produce 0.
> > vcvt_u32_f64(double_scratch.low(), input_reg, kFPSCRRounding);
> > Vmov(result_reg, double_scratch.low());
> > 
> > This is more-or-less equivalent to what we did in ClampDoubleToUint8 in
> > src/arm64/macro-assembler-arm64.cc, though the available instructions make
it
> > much simpler in A64.
> 
> This would be an excellent solution, but it seems the FPSCR state can be wrong
> when entering this function. VFPEnsureFPSCRState doesn't currently set the
> rounding mode, and I suppose it can be messed with by outside code. But if
> setting the FPSCR state in VFPEnsureFPSCRState will be enough, this solution
> could work.

The rounding mode should be round to nearest or other arithmetic operations are
potentially incorrect. You could add a check in debug mode to
VFPEnsureFPSCRState to check the rounding mode is round to nearest.

[jbramley, 2014-04-04 09:43:05]

On 2014/04/03 18:29:20, Rodolph Perfetta wrote:
> On 2014/04/03 16:02:45, oetuaho-nv wrote:
> > Thanks for the quick review, one comment inline.
> > 
> > I can look into this alternative solution and submit a new version once I
have
> > code and test results ready, and submit the assembler bug fix separately.
> > 
> >
>
https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm.cc
> > File src/arm/macro-assembler-arm.cc (left):
> > 
> >
>
https://codereview.chromium.org/222403002/diff/1/src/arm/macro-assembler-arm....
> > src/arm/macro-assembler-arm.cc:3819: // Set rounding mode to round to the
> > nearest integer by clearing bits[23:22].
> > On 2014/04/03 09:22:48, jbramley wrote:
> > > Shouldn't we be in the right rounding mode already? ECMAScript maths
> > operations
> > > are supposed to be done in round-to-nearest mode, and this is also the
> default
> > > FPSCR setting. If we've changed it explicitly somewhere else (for whatever
> > > reason), we're probably not doing normal ECMAScript maths properly.
> > > 
> > > If I'm correct about that, the whole thing collapses down:
> > > 
> > > // Handle inputs >= 255 (including +infinity).
> > > mov(result_reg, 255);
> > > Vmov(double_scratch, 255.0, result_reg);
> > > VFPCompareAndSetFlags(input_reg, double_scratch);
> > > b(ge, &done);
> > > 
> > > // All other inputs will clamp to the range [0-255]: NaN and -infinity
both
> > > produce 0.
> > > vcvt_u32_f64(double_scratch.low(), input_reg, kFPSCRRounding);
> > > Vmov(result_reg, double_scratch.low());
> > > 
> > > This is more-or-less equivalent to what we did in ClampDoubleToUint8 in
> > > src/arm64/macro-assembler-arm64.cc, though the available instructions make
> it
> > > much simpler in A64.
> > 
> > This would be an excellent solution, but it seems the FPSCR state can be
wrong
> > when entering this function. VFPEnsureFPSCRState doesn't currently set the
> > rounding mode, and I suppose it can be messed with by outside code. But if
> > setting the FPSCR state in VFPEnsureFPSCRState will be enough, this solution
> > could work.

I can't find any use of vmsr that sets the rounding mode to anything else. A few
tests hit it, but the rounding mode is correct in every case. Under what
conditions can FPSCR be wrong here?

> The rounding mode should be round to nearest or other arithmetic operations
are
> potentially incorrect. You could add a check in debug mode to
> VFPEnsureFPSCRState to check the rounding mode is round to nearest.

That sounds like a good idea.

[oetuaho-nv, 2014-04-04 14:14:17]

On 2014/04/04 09:43:05, jbramley wrote:
> I can't find any use of vmsr that sets the rounding mode to anything else. A
few
> tests hit it, but the rounding mode is correct in every case. Under what
> conditions can FPSCR be wrong here?

Yes, it's not being set by other code generated by v8, but is there anything
preventing other code that runs in the same execution context setting the FPSCR
to another value?

I wasn't able to verify this yet, since I'm unable to build android_arm.debug at
the moment, seems like due to https://code.google.com/p/v8/issues/detail?id=3258
. But I can get on it as soon as the build issue is fixed. I did get some
results running d8 release build on Android that would point to this direction,
but it would be better to verify with a debug mode check.

[jbramley, 2014-04-04 14:34:42]

On 2014/04/04 14:14:17, oetuaho-nv wrote:
> On 2014/04/04 09:43:05, jbramley wrote:
> > I can't find any use of vmsr that sets the rounding mode to anything
> > else. A few tests hit it, but the rounding mode is correct in every
> > case. Under what conditions can FPSCR be wrong here?
> 
> Yes, it's not being set by other code generated by v8, but is there
> anything preventing other code that runs in the same execution context
> setting the FPSCR to another value?

Sort of, yes. AAPCS (the procedure-call standard) says that the
rounding-mode bits may be modified by "specific support functions that
affect the global state of the application". That is, we would have to
do it explicitly somewhere.

[oetuaho-nv, 2014-04-04 15:16:07]

On 2014/04/04 14:34:42, jbramley wrote:
> On 2014/04/04 14:14:17, oetuaho-nv wrote:
> > On 2014/04/04 09:43:05, jbramley wrote:
> > > I can't find any use of vmsr that sets the rounding mode to anything
> > > else. A few tests hit it, but the rounding mode is correct in every
> > > case. Under what conditions can FPSCR be wrong here?
> > 
> > Yes, it's not being set by other code generated by v8, but is there
> > anything preventing other code that runs in the same execution context
> > setting the FPSCR to another value?
> 
> Sort of, yes. AAPCS (the procedure-call standard) says that the
> rounding-mode bits may be modified by "specific support functions that
> affect the global state of the application". That is, we would have to
> do it explicitly somewhere.

To me, that reads as you can't rely on the bits being set a certain way, unless
I'm understanding "global state of the application" wrong somehow. Isn't this
state shared with the entire renderer main thread in the case of Chromium, for
example, which is running all sorts of code?

I'm also curious about the NaN bit, according to AAPCS bit 25 is one of the
"other" bits that must not be modified, yet it is modified in
VFPEnsureFPSCRState?

[jbramley, 2014-04-04 15:36:56]

On 2014/04/04 15:16:07, oetuaho-nv wrote:
> On 2014/04/04 14:34:42, jbramley wrote:
> > On 2014/04/04 14:14:17, oetuaho-nv wrote:
> > > On 2014/04/04 09:43:05, jbramley wrote:
> > > > I can't find any use of vmsr that sets the rounding mode to
> > > > anything else. A few tests hit it, but the rounding mode is
> > > > correct in every case. Under what conditions can FPSCR be wrong
> > > > here?
> > > 
> > > Yes, it's not being set by other code generated by v8, but is
> > > there anything preventing other code that runs in the same
> > > execution context setting the FPSCR to another value?
> > 
> > Sort of, yes. AAPCS (the procedure-call standard) says that the
> > rounding-mode bits may be modified by "specific support functions
> > that affect the global state of the application". That is, we would
> > have to do it explicitly somewhere.
> 
> To me, that reads as you can't rely on the bits being set a certain
> way, unless I'm understanding "global state of the application" wrong
> somehow. Isn't this state shared with the entire renderer main thread
> in the case of Chromium, for example, which is running all sorts of
> code?

Yes, indeed, but by the same logic, if a project (such as Chromium)
wants to change the arithmetic settings, it must ensure that this change
is acceptable for its components (such as V8).

An assertion in VFPEnsureFPSCRState would be sufficient to enforce this,
I think.

> I'm also curious about the NaN bit, according to AAPCS bit 25 is one
> of the "other" bits that must not be modified, yet it is modified in
> VFPEnsureFPSCRState?

Yes, I wondered about that too. We modify it because we rely on
default-NaN mode to optimize some NaN canonicalization operations. AAPCS
says that this is non-compliant, but in this case it is probably a
worthwhile non-compliance.

We restore it when returning to (or calling) C++ code so the impact will
be limited to signal handlers. In general, C++ code doesn't care much
about default-NaN mode. This could be a problem later, but I think it's
quite unlikely.

[oetuaho-nv, 2014-04-09 14:10:30]

I now uploaded a new cl that implements the suggested approach:
https://codereview.chromium.org/230473005

I'm closing this one.