[wasm] Detect unrepresentability in the float32-to-int32 conversion correctly on arm.

[wasm] Detect unrepresentability in the float32-to-int32 conversion correctly on arm.

In the current implementation of wasm an unrepresentable input of the
float32-to-int32 conversion is detected by first truncating the input, then
converting the truncated input to int32 and back to float32, and then checking
whether the result is the same as the truncated input.

This input check does not work on arm and arm64 for an input of (INT32_MAX + 1)
because on these platforms the float32-to-int32 conversion results in INT32_MAX
if the input is greater than INT32_MAX.  When INT32_MAX is converted back to
float32, then the result is (INT32_MAX + 1) again because INT32_MAX cannot be
represented precisely as float32, and rounding-to-nearest results in (INT32_MAX
+ 1). Since (INT32_MAX + 1) equals the truncated input value, the input appears
to be representable.

With the changes in this CL, the result of the float32-to-int32 conversion is
incremented by 1 if the original result was INT32_MAX. Thereby the detection of
unrepresenable inputs in wasm works. Note that since INT32_MAX cannot be
represented precisely in float32, it can also never be a valid result of the
float32-to-int32 conversion.

@v8-mips-ports, can you do a similar implementation for mips?

R=titzer@chromium.org, Rodolph.Perfetta@arm.com

Committed: https://crrev.com/de369129d272c9cbd2a6282f4097a5d0ce264e85
Cr-Commit-Position: refs/heads/master@{#37448}

[Rodolph Perfetta (ARM), 2016-06-29 14:45:09]

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:1127: __ add(i.OutputRegister(),
i.OutputRegister(), Operand(1), SBit::LeaveCC,
on arm32 INT32_MIN is a valid immediate so the following code is also one
instructions but removes the dependency on the input:

  __ mov(i.OutputRegister(), Operand(INT32_MIN), SBit::LeaveCC, vs);

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:1139: __ add(i.OutputRegister(),
i.OutputRegister(), Operand(1), SBit::LeaveCC,
using adc would remove the dependency on the flags:

  __ adc(i.OutputRegister(), i.OutputRegister(), Operand::Zero());

https://codereview.chromium.org/2105313002/diff/20001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/2105313002/diff/20001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1340: __ Csinc(i.OutputRegister32(),
i.OutputRegister32(), i.OutputRegister32(),
as per arm32:

  __ Adc(i.OutputRegister(), i.OutputRegister(), Operand::Zero());

[ahaas, 2016-06-29 14:49:42]

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:1139: __ add(i.OutputRegister(),
i.OutputRegister(), Operand(1), SBit::LeaveCC,
On 2016/06/29 at 14:45:08, Rodolph Perfetta (ARM) wrote:
> using adc would remove the dependency on the flags:
> 
>   __ adc(i.OutputRegister(), i.OutputRegister(), Operand::Zero());

Is adc better than the mov instruction we use for the signed implementation,
e.g. __ mov(i.OutputRegister(), Operand(0), SBit::LeaveCC, cs)?

[Rodolph Perfetta (ARM), 2016-06-29 14:56:23]

> Is adc better than the mov instruction we use for the signed implementation,
> e.g. __ mov(i.OutputRegister(), Operand(0), SBit::LeaveCC, cs)?

conditional instructions are usually more expensive. So in order we have mov
immediate < add immediate < mov immediate conditional < add immediate
conditional.

[ahaas, 2016-06-29 15:07:33]

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
File src/compiler/arm/code-generator-arm.cc (right):

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:1127: __ add(i.OutputRegister(),
i.OutputRegister(), Operand(1), SBit::LeaveCC,
On 2016/06/29 at 14:45:08, Rodolph Perfetta (ARM) wrote:
> on arm32 INT32_MIN is a valid immediate so the following code is also one
instructions but removes the dependency on the input:
> 
>   __ mov(i.OutputRegister(), Operand(INT32_MIN), SBit::LeaveCC, vs);

Done.

https://codereview.chromium.org/2105313002/diff/1/src/compiler/arm/code-gener...
src/compiler/arm/code-generator-arm.cc:1139: __ add(i.OutputRegister(),
i.OutputRegister(), Operand(1), SBit::LeaveCC,
On 2016/06/29 at 14:45:08, Rodolph Perfetta (ARM) wrote:
> using adc would remove the dependency on the flags:
> 
>   __ adc(i.OutputRegister(), i.OutputRegister(), Operand::Zero());

Done.

https://codereview.chromium.org/2105313002/diff/20001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/2105313002/diff/20001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1340: __ Csinc(i.OutputRegister32(),
i.OutputRegister32(), i.OutputRegister32(),
On 2016/06/29 at 14:45:09, Rodolph Perfetta (ARM) wrote:
> as per arm32:
> 
>   __ Adc(i.OutputRegister(), i.OutputRegister(), Operand::Zero());

Done.

[Rodolph Perfetta (ARM), 2016-06-29 15:42:18]

LGTM with the comment addressed

https://codereview.chromium.org/2105313002/diff/40001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/2105313002/diff/40001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1340: __ Adc(i.OutputRegister(),
i.OutputRegister(), Operand(0));
i.OutputRegister32(), I made the mistake in my previous suggestion.

[ahaas, 2016-06-29 15:49:53]

https://codereview.chromium.org/2105313002/diff/40001/src/compiler/arm64/code...
File src/compiler/arm64/code-generator-arm64.cc (right):

https://codereview.chromium.org/2105313002/diff/40001/src/compiler/arm64/code...
src/compiler/arm64/code-generator-arm64.cc:1340: __ Adc(i.OutputRegister(),
i.OutputRegister(), Operand(0));
On 2016/06/29 at 15:42:18, Rodolph Perfetta (ARM) wrote:
> i.OutputRegister32(), I made the mistake in my previous suggestion.

Done. Thanks for looking at it so carefully again.

[titzer, 2016-06-30 10:55:26]

https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4037: if (input <
(static_cast<float>(static_cast<uint64_t>(UINT32_MAX) + 1))) {
Why don't we just spell out the constant as a float instead of all these static
casts? I am not sure if the C++ compiler has any leeway when casting out of the
representable range...I don't trust it.

[ahaas, 2016-06-30 12:02:27]

https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
test/cctest/compiler/test-run-machops.cc:4037: if (input <
(static_cast<float>(static_cast<uint64_t>(UINT32_MAX) + 1))) {
On 2016/06/30 at 10:55:26, titzer wrote:
> Why don't we just spell out the constant as a float instead of all these
static casts? I am not sure if the C++ compiler has any leeway when casting out
of the representable range...I don't trust it.

I wrote it this way because I think it is more readable and robust than spelling
it out as a float. The C++ should not have any leeway here because all values
here are in representable range: UINT32_MAX and also (UINT32_MAX + 1) are within
uint64 range, and (UINT32_MAX + 1) is representable as float.

[titzer, 2016-06-30 12:03:58]

On 2016/06/30 12:02:27, ahaas wrote:
>
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> File test/cctest/compiler/test-run-machops.cc (right):
> 
>
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> test/cctest/compiler/test-run-machops.cc:4037: if (input <
> (static_cast<float>(static_cast<uint64_t>(UINT32_MAX) + 1))) {
> On 2016/06/30 at 10:55:26, titzer wrote:
> > Why don't we just spell out the constant as a float instead of all these
> static casts? I am not sure if the C++ compiler has any leeway when casting
out
> of the representable range...I don't trust it.
> 
> I wrote it this way because I think it is more readable and robust than
spelling
> it out as a float. The C++ should not have any leeway here because all values
> here are in representable range: UINT32_MAX and also (UINT32_MAX + 1) are
within
> uint64 range, and (UINT32_MAX + 1) is representable as float.

UINT32_MAX + 1 is not representable in 32-bit float without rounding.

[ahaas, 2016-06-30 12:13:00]

On 2016/06/30 at 12:03:58, titzer wrote:
> On 2016/06/30 12:02:27, ahaas wrote:
> >
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> > File test/cctest/compiler/test-run-machops.cc (right):
> > 
> >
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> > test/cctest/compiler/test-run-machops.cc:4037: if (input <
> > (static_cast<float>(static_cast<uint64_t>(UINT32_MAX) + 1))) {
> > On 2016/06/30 at 10:55:26, titzer wrote:
> > > Why don't we just spell out the constant as a float instead of all these
> > static casts? I am not sure if the C++ compiler has any leeway when casting
out
> > of the representable range...I don't trust it.
> > 
> > I wrote it this way because I think it is more readable and robust than
spelling
> > it out as a float. The C++ should not have any leeway here because all
values
> > here are in representable range: UINT32_MAX and also (UINT32_MAX + 1) are
within
> > uint64 range, and (UINT32_MAX + 1) is representable as float.
> 
> UINT32_MAX + 1 is not representable in 32-bit float without rounding.

(UINT32_MAX + 1) is 0x100000000, in other words 1 * 2^32. It can be represented
precisely as float32. UINT32_MAX cannot be represented as float32, which is the
reason why I make the detour over uint64.

[titzer, 2016-06-30 12:20:05]

On 2016/06/30 12:13:00, ahaas wrote:
> On 2016/06/30 at 12:03:58, titzer wrote:
> > On 2016/06/30 12:02:27, ahaas wrote:
> > >
>
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> > > File test/cctest/compiler/test-run-machops.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> > > test/cctest/compiler/test-run-machops.cc:4037: if (input <
> > > (static_cast<float>(static_cast<uint64_t>(UINT32_MAX) + 1))) {
> > > On 2016/06/30 at 10:55:26, titzer wrote:
> > > > Why don't we just spell out the constant as a float instead of all these
> > > static casts? I am not sure if the C++ compiler has any leeway when
casting
> out
> > > of the representable range...I don't trust it.
> > > 
> > > I wrote it this way because I think it is more readable and robust than
> spelling
> > > it out as a float. The C++ should not have any leeway here because all
> values
> > > here are in representable range: UINT32_MAX and also (UINT32_MAX + 1) are
> within
> > > uint64 range, and (UINT32_MAX + 1) is representable as float.
> > 
> > UINT32_MAX + 1 is not representable in 32-bit float without rounding.
> 
> (UINT32_MAX + 1) is 0x100000000, in other words 1 * 2^32. It can be
represented
> precisely as float32. UINT32_MAX cannot be represented as float32, which is
the
> reason why I make the detour over uint64.

Ah, right. In that case I would prefer just writing it out, because none of that
is immediately obvious.

[ahaas, 2016-06-30 13:56:12]

On 2016/06/30 at 12:20:05, titzer wrote:
> On 2016/06/30 12:13:00, ahaas wrote:
> > On 2016/06/30 at 12:03:58, titzer wrote:
> > > On 2016/06/30 12:02:27, ahaas wrote:
> > > >
> >
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> > > > File test/cctest/compiler/test-run-machops.cc (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2105313002/diff/60001/test/cctest/compiler/te...
> > > > test/cctest/compiler/test-run-machops.cc:4037: if (input <
> > > > (static_cast<float>(static_cast<uint64_t>(UINT32_MAX) + 1))) {
> > > > On 2016/06/30 at 10:55:26, titzer wrote:
> > > > > Why don't we just spell out the constant as a float instead of all
these
> > > > static casts? I am not sure if the C++ compiler has any leeway when
casting
> > out
> > > > of the representable range...I don't trust it.
> > > > 
> > > > I wrote it this way because I think it is more readable and robust than
> > spelling
> > > > it out as a float. The C++ should not have any leeway here because all
> > values
> > > > here are in representable range: UINT32_MAX and also (UINT32_MAX + 1)
are
> > within
> > > > uint64 range, and (UINT32_MAX + 1) is representable as float.
> > > 
> > > UINT32_MAX + 1 is not representable in 32-bit float without rounding.
> > 
> > (UINT32_MAX + 1) is 0x100000000, in other words 1 * 2^32. It can be
represented
> > precisely as float32. UINT32_MAX cannot be represented as float32, which is
the
> > reason why I make the detour over uint64.
> 
> Ah, right. In that case I would prefer just writing it out, because none of
that is immediately obvious.

I added bound variables now in test-run-machops.cc, test-run-wasm.cc, and
wasm-interpreter.cc to make the code more readable.

[titzer, 2016-06-30 13:57:13]

lgtm

[ahaas, 2016-06-30 13:57:51]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-06-30 13:57:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
rodolph.perfetta@arm.com
Link to the patchset: https://codereview.chromium.org/2105313002/#ps80001
(title: "Define the bounds properly in variables.")

[commit-bot: I haz the power, 2016-06-30 13:57:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 14:29:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-30 14:30:52]

Description was changed from

==========
[wasm] Detect unrepresentability in the float32-to-int32 conversion correctly on
arm.

In the current implementation of wasm an unrepresentable input of the
float32-to-int32 conversion is detected by first truncating the input, then
converting the truncated input to int32 and back to float32, and then checking
whether the result is the same as the truncated input.

This input check does not work on arm and arm64 for an input of (INT32_MAX + 1)
because on these platforms the float32-to-int32 conversion results in INT32_MAX
if the input is greater than INT32_MAX.  When INT32_MAX is converted back to
float32, then the result is (INT32_MAX + 1) again because INT32_MAX cannot be
represented precisely as float32, and rounding-to-nearest results in (INT32_MAX
+ 1). Since (INT32_MAX + 1) equals the truncated input value, the input appears
to be representable.

With the changes in this CL, the result of the float32-to-int32 conversion is
incremented by 1 if the original result was INT32_MAX. Thereby the detection of
unrepresenable inputs in wasm works. Note that since INT32_MAX cannot be
represented precisely in float32, it can also never be a valid result of the
float32-to-int32 conversion.

@v8-mips-ports, can you do a similar implementation for mips?

R=titzer@chromium.org, Rodolph.Perfetta@arm.com
==========

to

==========
[wasm] Detect unrepresentability in the float32-to-int32 conversion correctly on
arm.

In the current implementation of wasm an unrepresentable input of the
float32-to-int32 conversion is detected by first truncating the input, then
converting the truncated input to int32 and back to float32, and then checking
whether the result is the same as the truncated input.

This input check does not work on arm and arm64 for an input of (INT32_MAX + 1)
because on these platforms the float32-to-int32 conversion results in INT32_MAX
if the input is greater than INT32_MAX.  When INT32_MAX is converted back to
float32, then the result is (INT32_MAX + 1) again because INT32_MAX cannot be
represented precisely as float32, and rounding-to-nearest results in (INT32_MAX
+ 1). Since (INT32_MAX + 1) equals the truncated input value, the input appears
to be representable.

With the changes in this CL, the result of the float32-to-int32 conversion is
incremented by 1 if the original result was INT32_MAX. Thereby the detection of
unrepresenable inputs in wasm works. Note that since INT32_MAX cannot be
represented precisely in float32, it can also never be a valid result of the
float32-to-int32 conversion.

@v8-mips-ports, can you do a similar implementation for mips?

R=titzer@chromium.org, Rodolph.Perfetta@arm.com

Committed: https://crrev.com/de369129d272c9cbd2a6282f4097a5d0ce264e85
Cr-Commit-Position: refs/heads/master@{#37448}
==========

[commit-bot: I haz the power, 2016-06-30 14:30:53]

Patchset 5 (id:??) landed as
https://crrev.com/de369129d272c9cbd2a6282f4097a5d0ce264e85
Cr-Commit-Position: refs/heads/master@{#37448}