[turbofan] IA: Float64ToUint32 supports mem operand

[turbofan] IA: Float64ToUint32 supports mem operand

BUG=
R=titzer@chromium.org

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

[Weiliang, 2014-09-18 13:57:53]

weiliang.lin@intel.com changed reviewers:
+ bmeurer@chromium.org, danno@chromium.org, titzer@chromium.org

[Weiliang, 2014-09-18 13:57:54]

PTAL

[titzer, 2014-09-18 14:03:06]

Looks good, just wondering about the below conditions.

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

https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
src/compiler/ia32/code-generator-ia32.cc:310: __ j(not_carry, &done,
Label::kNear);
What does this extra condition here check?

Note that this the implementation of ChangeFloat64ToUint32, which explicitly
assumes that all of its inputs lie in the range [0...2^32-1] and are integers.

[Weiliang, 2014-09-18 14:11:01]

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

https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
src/compiler/ia32/code-generator-ia32.cc:310: __ j(not_carry, &done,
Label::kNear);
On 2014/09/18 14:03:06, titzer wrote:
> What does this extra condition here check?
> 
> Note that this the implementation of ChangeFloat64ToUint32, which explicitly
> assumes that all of its inputs lie in the range [0...2^32-1] and are integers.

cvttsd2si truncates the result by rounding toward zero. So when changing the
range from [0...2^32-1] to [-2^31, 2^31-1], we need to compensate the result.

[Weiliang, 2014-09-18 14:31:21]

On 2014/09/18 14:11:01, Weiliang wrote:
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> File src/compiler/ia32/code-generator-ia32.cc (right):
> 
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> src/compiler/ia32/code-generator-ia32.cc:310: __ j(not_carry, &done,
> Label::kNear);
> On 2014/09/18 14:03:06, titzer wrote:
> > What does this extra condition here check?
> > 
> > Note that this the implementation of ChangeFloat64ToUint32, which explicitly
> > assumes that all of its inputs lie in the range [0...2^32-1] and are
integers.
> 
> cvttsd2si truncates the result by rounding toward zero. So when changing the
> range from [0...2^32-1] to [-2^31, 2^31-1], we need to compensate the result.

Well, i don't see your comment quite clearly. 

If the input lies in the range [0...2^32-1] and are integers, the check is
redundant I think.(So the test case should be re-written to only feed integers.)
But could you please share where does such explicit assumption come from?

Thanks
-Weiliang

[titzer, 2014-09-18 15:46:16]

On 2014/09/18 14:31:21, Weiliang wrote:
> On 2014/09/18 14:11:01, Weiliang wrote:
> >
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> > File src/compiler/ia32/code-generator-ia32.cc (right):
> > 
> >
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> > src/compiler/ia32/code-generator-ia32.cc:310: __ j(not_carry, &done,
> > Label::kNear);
> > On 2014/09/18 14:03:06, titzer wrote:
> > > What does this extra condition here check?
> > > 
> > > Note that this the implementation of ChangeFloat64ToUint32, which
explicitly
> > > assumes that all of its inputs lie in the range [0...2^32-1] and are
> integers.
> > 
> > cvttsd2si truncates the result by rounding toward zero. So when changing the
> > range from [0...2^32-1] to [-2^31, 2^31-1], we need to compensate the
result.
> 
> Well, i don't see your comment quite clearly. 
> 
> If the input lies in the range [0...2^32-1] and are integers, the check is
> redundant I think.(So the test case should be re-written to only feed
integers.)
> But could you please share where does such explicit assumption come from?
> 

This is by design. All operators (both simplified and machine) that have Change
in the name only change representations, and don't do truncations. In this case,
it's not easy to tell that SSEFloat64ToInt32 should have assume this. But you're
right, the test should only feed integers in the appropriate range.

> Thanks
> -Weiliang

[Weiliang, 2014-09-18 16:04:11]

On 2014/09/18 15:46:16, titzer wrote:
> On 2014/09/18 14:31:21, Weiliang wrote:
> > On 2014/09/18 14:11:01, Weiliang wrote:
> > >
> >
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> > > File src/compiler/ia32/code-generator-ia32.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> > > src/compiler/ia32/code-generator-ia32.cc:310: __ j(not_carry, &done,
> > > Label::kNear);
> > > On 2014/09/18 14:03:06, titzer wrote:
> > > > What does this extra condition here check?
> > > > 
> > > > Note that this the implementation of ChangeFloat64ToUint32, which
> explicitly
> > > > assumes that all of its inputs lie in the range [0...2^32-1] and are
> > integers.
> > > 
> > > cvttsd2si truncates the result by rounding toward zero. So when changing
the
> > > range from [0...2^32-1] to [-2^31, 2^31-1], we need to compensate the
> result.
> > 
> > Well, i don't see your comment quite clearly. 
> > 
> > If the input lies in the range [0...2^32-1] and are integers, the check is
> > redundant I think.(So the test case should be re-written to only feed
> integers.)
> > But could you please share where does such explicit assumption come from?
> > 
> 
> This is by design. All operators (both simplified and machine) that have
Change
> in the name only change representations, and don't do truncations. In this
case,
> it's not easy to tell that SSEFloat64ToInt32 should have assume this. But
you're
> right, the test should only feed integers in the appropriate range.
> 
> > Thanks
> > -Weiliang

Thanks for sharing. Please take a look at the second patch set. 
Thanks
-Weiliang

[titzer, 2014-09-18 16:12:46]

On 2014/09/18 16:04:11, Weiliang wrote:
> On 2014/09/18 15:46:16, titzer wrote:
> > On 2014/09/18 14:31:21, Weiliang wrote:
> > > On 2014/09/18 14:11:01, Weiliang wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> > > > File src/compiler/ia32/code-generator-ia32.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/582713002/diff/1/src/compiler/ia32/code-gener...
> > > > src/compiler/ia32/code-generator-ia32.cc:310: __ j(not_carry, &done,
> > > > Label::kNear);
> > > > On 2014/09/18 14:03:06, titzer wrote:
> > > > > What does this extra condition here check?
> > > > > 
> > > > > Note that this the implementation of ChangeFloat64ToUint32, which
> > explicitly
> > > > > assumes that all of its inputs lie in the range [0...2^32-1] and are
> > > integers.
> > > > 
> > > > cvttsd2si truncates the result by rounding toward zero. So when changing
> the
> > > > range from [0...2^32-1] to [-2^31, 2^31-1], we need to compensate the
> > result.
> > > 
> > > Well, i don't see your comment quite clearly. 
> > > 
> > > If the input lies in the range [0...2^32-1] and are integers, the check is
> > > redundant I think.(So the test case should be re-written to only feed
> > integers.)
> > > But could you please share where does such explicit assumption come from?
> > > 
> > 
> > This is by design. All operators (both simplified and machine) that have
> Change
> > in the name only change representations, and don't do truncations. In this
> case,
> > it's not easy to tell that SSEFloat64ToInt32 should have assume this. But
> you're
> > right, the test should only feed integers in the appropriate range.
> > 
> > > Thanks
> > > -Weiliang
> 
> Thanks for sharing. Please take a look at the second patch set. 
> Thanks
> -Weiliang

lgtm

[titzer, 2014-09-19 14:25:28]

Committed patchset #2 manually as 24092 (tree was closed).