[ia32/x64] Smaller instruction to check NaN

[ia32/x64] Smaller instruction to check NaN

substract 1 and test for overflow

BUG=
R=jkummerow@chromium.org

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

[Jakob Kummerow, 2014-03-17 18:06:46]

Do we have test coverage for all code paths that have changed? It's easy to
introduce a typo when refactoring jump conditions, accidentally reversing a
condition...

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

https://codereview.chromium.org/202083002/diff/1/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:3076: j(below_equal, &done, Label::kNear);
This change is confusing. "zero" should have the same effect as "below_equal" in
this case, and is much more readable. Or am I missing something?

[Weiliang, 2014-03-18 06:06:40]

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

https://codereview.chromium.org/202083002/diff/1/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:3076: j(below_equal, &done, Label::kNear);
Oh, this is a stale change which I forget to change back. Sorry for that.
On 2014/03/17 18:06:46, Jakob wrote:
> This change is confusing. "zero" should have the same effect as "below_equal"
in
> this case, and is much more readable. Or am I missing something?

https://codereview.chromium.org/202083002/diff/1/src/x64/macro-assembler-x64....
src/x64/macro-assembler-x64.cc:3076: j(below_equal, &done, Label::kNear);
On 2014/03/17 18:06:46, Jakob wrote:
> This change is confusing. "zero" should have the same effect as "below_equal"
in
> this case, and is much more readable. Or am I missing something?

Done.

[Weiliang, 2014-03-19 07:24:28]

On 2014/03/17 18:06:46, Jakob wrote:
> Do we have test coverage for all code paths that have changed? It's easy to
> introduce a typo when refactoring jump conditions, accidentally reversing a
> condition...
> 
>
https://codereview.chromium.org/202083002/diff/1/src/x64/macro-assembler-x64.cc
> File src/x64/macro-assembler-x64.cc (right):
> 
>
https://codereview.chromium.org/202083002/diff/1/src/x64/macro-assembler-x64....
> src/x64/macro-assembler-x64.cc:3076: j(below_equal, &done, Label::kNear);
> This change is confusing. "zero" should have the same effect as "below_equal"
in
> this case, and is much more readable. Or am I missing something?

Hi Jakob,
I assume there are test coverage for the whole code paths. But I am not sure
because the changes occurs in quite a lot different path although I changed it
very carefully. 
It seems time consuming to construct a test case to cover them all. But I could
do it if it is necessary for landed.

[Jakob Kummerow, 2014-03-19 12:00:30]

Well, rule #1 of V8 test coverage: never assume that it exists.
A quick way to find out is to insert an int3 instruction into the changed code
path and see if any test hits it. Please do add tests for any paths that are not
covered yet -- yes, that takes time, but it's important for the project!

Also, please split the change to the ClampToUint8 behavior (returning more than
8 bits) into a different CL, as that is unrelated to the NaN checks and deserves
its own discussion. While I agree that current uses of that code sequence seem
safe, I'm not convinced it's acceptable to change the contract in such a
potentially surprising way: with your changes, 256 is "clamped" to -1 rather
than 255 when interpreting the bit pattern as the Integer32 output
representation indicates.

https://codereview.chromium.org/202083002/diff/20001/src/ia32/macro-assembler...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/202083002/diff/20001/src/ia32/macro-assembler...
src/ia32/macro-assembler-ia32.cc:224: not_(result_reg);  // safe not to zero
extension
This comment doesn't make any sense, unless the reader knows that in one of the
places where you're putting it there used to be a movzx_b instruction.
Also, comments should be complete sentences with proper grammar, capitalization
and punctuation.

[Weiliang, 2014-03-21 15:04:17]

On 2014/03/19 12:00:30, Jakob wrote:
> Well, rule #1 of V8 test coverage: never assume that it exists.
> A quick way to find out is to insert an int3 instruction into the changed code
> path and see if any test hits it. Please do add tests for any paths that are
not
> covered yet -- yes, that takes time, but it's important for the project!
> 
> Also, please split the change to the ClampToUint8 behavior (returning more
than
> 8 bits) into a different CL, as that is unrelated to the NaN checks and
deserves
> its own discussion. While I agree that current uses of that code sequence seem
> safe, I'm not convinced it's acceptable to change the contract in such a
> potentially surprising way: with your changes, 256 is "clamped" to -1 rather
> than 255 when interpreting the bit pattern as the Integer32 output
> representation indicates.
> 
>
https://codereview.chromium.org/202083002/diff/20001/src/ia32/macro-assembler...
> File src/ia32/macro-assembler-ia32.cc (right):
> 
>
https://codereview.chromium.org/202083002/diff/20001/src/ia32/macro-assembler...
> src/ia32/macro-assembler-ia32.cc:224: not_(result_reg);  // safe not to zero
> extension
> This comment doesn't make any sense, unless the reader knows that in one of
the
> places where you're putting it there used to be a movzx_b instruction.
> Also, comments should be complete sentences with proper grammar,
capitalization
> and punctuation.

Hi Jakob
Good idea to test the code coverage by int3. I have tested all code change sites
and found all paths are covered. So it is safe to land this CL. And I also split
the ClampToUint8 into another CL. I will submit it after this CL is landed. 

Thanks a lot.

[Jakob Kummerow, 2014-03-24 09:27:46]

LGTM, I'll land.

[Jakob Kummerow, 2014-03-24 10:18:36]

Committed patchset #3 manually as r20185 (presubmit successful).