Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2721053002:
MIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs. (Closed)
|
Created:
3 years, 9 months ago by ivica.bogosavljevic Modified:
3 years, 9 months ago CC:
v8-mips-ports_googlegroups.com, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs.
WASM interpreter requires that parameters are stored in big-endian natural
memory order (higher bits on lower addresses and lower bits on higher address).
On the other hand, WASM compiled code naturally stores data in memory in
little-endian order. This CL implements big-endian support for passing
double and int64 parameters to WASM interpreter.
TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_int64,
cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes
BUG=v8:6043
Review-Url: https://codereview.chromium.org/2721053002
Cr-Commit-Position: refs/heads/master@{#43568}
Committed: https://chromium.googlesource.com/v8/v8/+/4f426e104dd67d2dd72bf44aef70c032eea3be7d
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address code review remarks #
Messages
Total messages: 35 (21 generated)
ivica.bogosavljevic@imgtec.com changed reviewers: + clemensh@google.com, titzer@chromium.org
PTAL
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2721053002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2721053002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2988: store_rep = StoreRepresentation(wasm::kWasmI32, This assignment is redundant. https://codereview.chromium.org/2721053002/diff/1/src/utils.h File src/utils.h (right): https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode523 src/utils.h:523: // Memory offset for Lower and Higher bits in a It looks like this comment could fit in one line. Also, please fix the capitalization of "Lower" and "Higher" and add a "." at the end. https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode525 src/utils.h:525: #if defined(V8_TARGET_LITTLE_ENDIAN) Please move these definitions to wasm-compiler.cc. https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode526 src/utils.h:526: static const int kInt64LowerBitsMememoryOffset = 0; Typo "Mememory"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2721053002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2721053002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2988: store_rep = StoreRepresentation(wasm::kWasmI32, On 2017/03/01 08:51:13, Clemens Hammacher wrote: > This assignment is redundant. Acknowledged. https://codereview.chromium.org/2721053002/diff/1/src/utils.h File src/utils.h (right): https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode523 src/utils.h:523: // Memory offset for Lower and Higher bits in a On 2017/03/01 08:51:13, Clemens Hammacher wrote: > It looks like this comment could fit in one line. Also, please fix the > capitalization of "Lower" and "Higher" and add a "." at the end. Acknowledged. https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode525 src/utils.h:525: #if defined(V8_TARGET_LITTLE_ENDIAN) On 2017/03/01 08:51:13, Clemens Hammacher wrote: > Please move these definitions to wasm-compiler.cc. I added this definition on purpose in the header and not in wasm-compiler.cc. There are same constants albeit with the different name defined elsewhere e.g. int64-lowering.cc:123, objects.h:1820, assembler-mips.h:115. I plan to upload another patch that cleans this up. https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode526 src/utils.h:526: static const int kInt64LowerBitsMememoryOffset = 0; On 2017/03/01 08:51:13, Clemens Hammacher wrote: > Typo "Mememory" Acknowledged.
clemensh@chromium.org changed reviewers: - clemensh@google.com
Can you upload the new patch set please? https://codereview.chromium.org/2721053002/diff/1/src/utils.h File src/utils.h (right): https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode525 src/utils.h:525: #if defined(V8_TARGET_LITTLE_ENDIAN) On 2017/03/01 at 12:58:48, ivica.bogosavljevic wrote: > On 2017/03/01 08:51:13, Clemens Hammacher wrote: > > Please move these definitions to wasm-compiler.cc. > > I added this definition on purpose in the header and not in wasm-compiler.cc. There are same constants albeit with the different name defined elsewhere e.g. int64-lowering.cc:123, objects.h:1820, assembler-mips.h:115. I plan to upload another patch that cleans this up. Agreed, that sounds reasonable. Looking forward to this cleanup. Could you pick a more descriptive name anyway? "HigherBits" and "LowerBits" sounds too generic for me (which bits?). What about "UpperHalf" and "LowerHalf"?
Uploaded a new CL, PTAL https://codereview.chromium.org/2721053002/diff/1/src/utils.h File src/utils.h (right): https://codereview.chromium.org/2721053002/diff/1/src/utils.h#newcode525 src/utils.h:525: #if defined(V8_TARGET_LITTLE_ENDIAN) On 2017/03/01 13:15:20, Clemens Hammacher wrote: > On 2017/03/01 at 12:58:48, ivica.bogosavljevic wrote: > > On 2017/03/01 08:51:13, Clemens Hammacher wrote: > > > Please move these definitions to wasm-compiler.cc. > > > > I added this definition on purpose in the header and not in wasm-compiler.cc. > There are same constants albeit with the different name defined elsewhere e.g. > int64-lowering.cc:123, objects.h:1820, assembler-mips.h:115. I plan to upload > another patch that cleans this up. > > Agreed, that sounds reasonable. Looking forward to this cleanup. > Could you pick a more descriptive name anyway? "HigherBits" and "LowerBits" > sounds too generic for me (which bits?). What about "UpperHalf" and "LowerHalf"? Acknowledged.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
MIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs.
WASM interpreter requires that parameters are stored in big-endian natural
memory order (higher bits on lower addresses and lower bits on higher address).
On the other hand, WASM compiled code naturally stores data in memory in
little-endian order. This CL implements big-endian support for passing
double and int64 parameters to WASM interpreter.
TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_int64,
cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes
BUG=
==========
to
==========
MIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs.
WASM interpreter requires that parameters are stored in big-endian natural
memory order (higher bits on lower addresses and lower bits on higher address).
On the other hand, WASM compiled code naturally stores data in memory in
little-endian order. This CL implements big-endian support for passing
double and int64 parameters to WASM interpreter.
TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_int64,
cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes
==========
LGTM, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488526376378570,
"parent_rev": "0832bae354f6001ba5b3647c42304bad1003bbaa", "commit_rev":
"4f426e104dd67d2dd72bf44aef70c032eea3be7d"}
Message was sent while issue was closed.
Description was changed from
==========
MIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs.
WASM interpreter requires that parameters are stored in big-endian natural
memory order (higher bits on lower addresses and lower bits on higher address).
On the other hand, WASM compiled code naturally stores data in memory in
little-endian order. This CL implements big-endian support for passing
double and int64 parameters to WASM interpreter.
TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_int64,
cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes
==========
to
==========
MIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs.
WASM interpreter requires that parameters are stored in big-endian natural
memory order (higher bits on lower addresses and lower bits on higher address).
On the other hand, WASM compiled code naturally stores data in memory in
little-endian order. This CL implements big-endian support for passing
double and int64 parameters to WASM interpreter.
TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_int64,
cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes
Review-Url: https://codereview.chromium.org/2721053002
Cr-Commit-Position: refs/heads/master@{#43568}
Committed:
https://chromium.googlesource.com/v8/v8/+/4f426e104dd67d2dd72bf44aef70c032eea...
==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/4f426e104dd67d2dd72bf44aef70c032eea...
Message was sent while issue was closed.
bjaideep@ca.ibm.com changed reviewers: + bjaideep@ca.ibm.com
Message was sent while issue was closed.
Hi, Can this be backported to 5.8? this also affects ppc/s390 big endian platforms. Thank you.
Message was sent while issue was closed.
Please follow https://github.com/v8/v8/wiki/Merging%20&%20Patching. Backporting this is lgtm.
Message was sent while issue was closed.
On 2017/03/06 at 09:58:48, hablich wrote: > Please follow https://github.com/v8/v8/wiki/Merging%20&%20Patching. > > Backporting this is lgtm. Bug here: https://bugs.chromium.org/p/v8/issues/detail?id=6043 Waiting for canary coverage.
Message was sent while issue was closed.
On 2017/03/06 10:20:54, Clemens Hammacher wrote: > On 2017/03/06 at 09:58:48, hablich wrote: > > Please follow https://github.com/v8/v8/wiki/Merging%20&%20Patching. > > > > Backporting this is lgtm. > > Bug here: https://bugs.chromium.org/p/v8/issues/detail?id=6043 > Waiting for canary coverage. Please reference the bug in the issue description.
Message was sent while issue was closed.
Description was changed from
==========
MIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs.
WASM interpreter requires that parameters are stored in big-endian natural
memory order (higher bits on lower addresses and lower bits on higher address).
On the other hand, WASM compiled code naturally stores data in memory in
little-endian order. This CL implements big-endian support for passing
double and int64 parameters to WASM interpreter.
TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_int64,
cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes
Review-Url: https://codereview.chromium.org/2721053002
Cr-Commit-Position: refs/heads/master@{#43568}
Committed:
https://chromium.googlesource.com/v8/v8/+/4f426e104dd67d2dd72bf44aef70c032eea...
==========
to
==========
MIPS: Fix int64->int32 lowering in wasm-to-interpeter entry on big-endian archs.
WASM interpreter requires that parameters are stored in big-endian natural
memory order (higher bits on lower addresses and lower bits on higher address).
On the other hand, WASM compiled code naturally stores data in memory in
little-endian order. This CL implements big-endian support for passing
double and int64 parameters to WASM interpreter.
TEST=cctest/test-wasm-interpreter-entry/TestArgumentPassing_int64,
cctest/test-wasm-interpreter-entry/TestArgumentPassing_AllTypes
BUG=v8:6043
Review-Url: https://codereview.chromium.org/2721053002
Cr-Commit-Position: refs/heads/master@{#43568}
Committed:
https://chromium.googlesource.com/v8/v8/+/4f426e104dd67d2dd72bf44aef70c032eea...
==========
Message was sent while issue was closed.
clemensh@chromium.org changed reviewers: + hablich@chromium.org
Message was sent while issue was closed.
On 2017/03/08 at 14:22:20, hablich wrote: > On 2017/03/06 10:20:54, Clemens Hammacher wrote: > > On 2017/03/06 at 09:58:48, hablich wrote: > > > Please follow https://github.com/v8/v8/wiki/Merging%20&%20Patching. > > > > > > Backporting this is lgtm. > > > > Bug here: https://bugs.chromium.org/p/v8/issues/detail?id=6043 > > Waiting for canary coverage. > > Please reference the bug in the issue description. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
