Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1433353006:
[machine-operator-reducer] fix float truncation (Closed)
|
Created:
5 years, 1 month ago by fedor.indutny Modified:
5 years ago Reviewers:
titzer CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[machine-operator-reducer] fix float truncation
Don't replace `TruncateFloat64ToInt32(RoundInt64ToFloat64(value))` with
`value`. Generally, `value` may have a range bigger than the one that
could fit into Int32. Replace it with `TruncateInt64ToInt32(value)`
instead, and only if the `value` fits into Float64 without precision
loss.
Add missing mjsunit test for 52bit multiplication/division optimization
that has landed in refs/heads/master@{#31899}.
BUG=
R=titzer@google.com
Committed: https://crrev.com/64efa2a904773816968992628f0bf0f1b7ae82be
Cr-Commit-Position: refs/heads/master@{#32227}
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixes #Patch Set 3 : fix arguments order for assertEquals #Patch Set 4 : arm64: do not merge 64bit shr/sar with 32bit op #
Total comments: 5
Patch Set 5 : fix #Patch Set 6 : fix the bug in machine-operator-reducer instead of fixing arm64 #Patch Set 7 : add test for machine-operator-reducer #
Total comments: 2
Patch Set 8 : fix typo #
Messages
Total messages: 30 (5 generated)
fedor@indutny.com changed reviewers: + titzer@chromium.org - titzer@google.com
Thanks for writing more tests. Mostly looks good modulo small comments. https://codereview.chromium.org/1433353006/diff/1/test/mjsunit/math-52-mul-di... File test/mjsunit/math-52-mul-div.js (right): https://codereview.chromium.org/1433353006/diff/1/test/mjsunit/math-52-mul-di... test/mjsunit/math-52-mul-div.js:1: // Copyright 2015 the V8 project authors. All rights reserved. You can use the short version of the copyright header here. https://codereview.chromium.org/1433353006/diff/1/test/mjsunit/math-52-mul-di... test/mjsunit/math-52-mul-div.js:71: function test(fn, a, b) { Can you add a few more inputs, e.g. some pseudo-randomly generated inputs with ground truth expectations?
All fixed. Thanks! https://codereview.chromium.org/1433353006/diff/1/test/mjsunit/math-52-mul-di... File test/mjsunit/math-52-mul-div.js (right): https://codereview.chromium.org/1433353006/diff/1/test/mjsunit/math-52-mul-di... test/mjsunit/math-52-mul-div.js:1: // Copyright 2015 the V8 project authors. All rights reserved. On 2015/11/11 21:04:43, titzer wrote: > You can use the short version of the copyright header here. Acknowledged. https://codereview.chromium.org/1433353006/diff/1/test/mjsunit/math-52-mul-di... test/mjsunit/math-52-mul-div.js:71: function test(fn, a, b) { On 2015/11/11 21:04:43, titzer wrote: > Can you add a few more inputs, e.g. some pseudo-randomly generated inputs with > ground truth expectations? Acknowledged.
lgtm
The CQ bit was checked by fedor@indutny.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433353006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433353006/20001
Looks like it does not work on arm64. Investigating.
Ben, It turns out to be a bug in arm64 instruction selector. I have included the fix. Please let me know if it should be altered. Thank you, Fedor.
To make it completely clear. This is what the generated arm64 code looks like before the fix: 0x1084074e0 96 mul x0, x1, x0 0x1084074e4 100 and w1, w0, #0x3ffffff 0x1084074e8 104 orr w0, w1, w0, lsr #26 Here it is right after the fix: 0x1064074e0 96 mul x0, x1, x0 0x1064074e4 100 lsr x1, x0, #26 0x1064074e8 104 and w0, w0, #0x3ffffff So basically what happens is that `orr` operates only on lower 32bits of its inputs, and embedded 26bit right shift does not return the bits from the upper 32bits. Hope this helps!
https://codereview.chromium.org/1433353006/diff/60001/src/compiler/arm64/inst... File src/compiler/arm64/instruction-selector-arm64.cc (right): https://codereview.chromium.org/1433353006/diff/60001/src/compiler/arm64/inst... src/compiler/arm64/instruction-selector-arm64.cc:153: bool is_word_output = !IrOpcode::Is64(node->opcode()); Can you move this condition inline (so that it is not evaluated for every case)? https://codereview.chromium.org/1433353006/diff/60001/src/compiler/opcodes.cc File src/compiler/opcodes.cc (right): https://codereview.chromium.org/1433353006/diff/60001/src/compiler/opcodes.cc... src/compiler/opcodes.cc:40: bool IrOpcode::Is64(Value value) { What are the semantics of this predicate? It's not quite clear, e.g. Float64Constants are listed here too.
https://codereview.chromium.org/1433353006/diff/60001/src/compiler/arm64/inst... File src/compiler/arm64/instruction-selector-arm64.cc (right): https://codereview.chromium.org/1433353006/diff/60001/src/compiler/arm64/inst... src/compiler/arm64/instruction-selector-arm64.cc:153: bool is_word_output = !IrOpcode::Is64(node->opcode()); On 2015/11/13 23:35:17, titzer wrote: > Can you move this condition inline (so that it is not evaluated for every case)? Acknowledged. https://codereview.chromium.org/1433353006/diff/60001/src/compiler/opcodes.cc File src/compiler/opcodes.cc (right): https://codereview.chromium.org/1433353006/diff/60001/src/compiler/opcodes.cc... src/compiler/opcodes.cc:40: bool IrOpcode::Is64(Value value) { This method returns true if the opcode output is a 64-bit value.
Hello! Should I change the description of the CL? (Also does it LGTY?) Thanks!
On 2015/11/16 16:47:15, fedor.indutny wrote: > Hello! > > Should I change the description of the CL? (Also does it LGTY?) > > Thanks! Yeah, you can update the description. Also, can you add a unittest for the arm64 instruction selector? Otherwise looks good, so you can land when you have the unittest.
Description was changed from
==========
[mjsunit] add test for refs/heads/master@{#31899}
Add missing mjsunit test for 52bit multiplication/division optimization
that has landed in refs/heads/master@{#31899}.
BUG=
R=titzer@google.com
==========
to
==========
[machine-operator-reducer] fix float truncation
Don't replace `TruncateFloat64ToInt32(RoundInt64ToFloat64(value))` with
`value`. Generally, `value` may have a range bigger than the one that
could fit into Int32. Replace it with `TruncateInt64ToInt32(value)`
instead, and only if the `value` fits into Float64 without precision
loss.
Add missing mjsunit test for 52bit multiplication/division optimization
that has landed in refs/heads/master@{#31899}.
BUG=
R=titzer@google.com
==========
On 2015/11/18 08:41:34, titzer wrote: > On 2015/11/16 16:47:15, fedor.indutny wrote: > > Hello! > > > > Should I change the description of the CL? (Also does it LGTY?) > > > > Thanks! > > Yeah, you can update the description. Also, can you add a unittest for the arm64 > instruction selector? Otherwise looks good, so you can land when you have the > unittest. While working on test I discovered that the issue is actually in a machine-operator-reducer.cc, and not in arm64. It should be truncating int64 to int32 instead of using the value as it is. May I ask you to take a look at the new diff? I'm slightly worried about using range in machine-operator-reducer.cc, but I need it to avoid the float conversion. Do you have any suggestions on how it could be improved? And one more thing: turns out `Revisit()` does nothing on newly added nodes, so there is no point in calling it in `binary-operator-reducer.cc`. Given that, I can probably merge it with machine-operator-reducer.cc in a separate CL. Does it sound reasonable? Thank you so much for your time, Fedor.
Ping.
https://codereview.chromium.org/1433353006/diff/60001/src/compiler/arm64/inst... File src/compiler/arm64/instruction-selector-arm64.cc (right): https://codereview.chromium.org/1433353006/diff/60001/src/compiler/arm64/inst... src/compiler/arm64/instruction-selector-arm64.cc:144: Node* input_node, InstructionCode* opcode, bool try_ror) { Can you add additional unittests for the instruction selector in test/unittests/compiler/arm64?
On 2015/11/23 23:55:03, fedor.indutny wrote: > Ping. Sorry, the below comment was sitting unsent for a week!
Sorry, but I think your comments no longer apply. It turned out that arm code generation doesn't need to be fixed. I have fixed the machine-operator-reducer instead. Could you please take a look? Thanks!
lgtm https://codereview.chromium.org/1433353006/diff/120001/src/compiler/machine-o... File src/compiler/machine-operator-reducer.cc (right): https://codereview.chromium.org/1433353006/diff/120001/src/compiler/machine-o... src/compiler/machine-operator-reducer.cc:656: // Rounding int64 to float64 should not loose precision s/loose/lose/
Fixed typo. Thanks! https://codereview.chromium.org/1433353006/diff/120001/src/compiler/machine-o... File src/compiler/machine-operator-reducer.cc (right): https://codereview.chromium.org/1433353006/diff/120001/src/compiler/machine-o... src/compiler/machine-operator-reducer.cc:656: // Rounding int64 to float64 should not loose precision On 2015/11/24 19:30:35, titzer wrote: > s/loose/lose/ Acknowledged.
The CQ bit was checked by fedor@indutny.com
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org Link to the patchset: https://codereview.chromium.org/1433353006/#ps140001 (title: "fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433353006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433353006/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/64efa2a904773816968992628f0bf0f1b7ae82be Cr-Commit-Position: refs/heads/master@{#32227}
Message was sent while issue was closed.
Hooray, thank you!
Message was sent while issue was closed.
I'm afraid but this is also unsound for the reasons outlined in https://codereview.chromium.org/1473073004/ I'll do some surgery and keep the useful mjsunit tests.
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1468313009/ by bmeurer@chromium.org. The reason for reverting is: This is also unsound for the reasons outlined in https://codereview.chromium.org/1473073004/ Will reland the mjsunit test separately and help Fedor to implement a solution based on simplified operators.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
