Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2764583003:
[wasm] Fix wasm interpreter entry for 32 bit big endian systems (Closed)
Description[wasm] Fix wasm interpreter entry for 32 bit big endian systems
The order of the return values are wrong for 32 bit big endian machines.
BUG=none
R=titzer@chromium.org, clemensh@chromium.org,
Review-Url: https://codereview.chromium.org/2764583003
Cr-Commit-Position: refs/heads/master@{#44041}
Committed: https://chromium.googlesource.com/v8/v8/+/04440d28693474cf0a7c10c5a63915528a227a5f
Patch Set 1 #
Total comments: 2
Patch Set 2 : using the kInt64LowerHalfMemoryOffset and kInt64UpperHalfMemoryOffset constants to load the lower and upper half #Messages
Total messages: 17 (9 generated)
ptal
Nit: Please write the commit message in present tense: "Fix ..." https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2852: #if defined(V8_TARGET_LITTLE_ENDIAN) Instead of introducing another "#if defined(...)", can you please use the kInt64LowerHalfMemoryOffset and kInt64UpperHalfMemoryOffset constants to load the lower and upper half, similar to the store nodes for the parameters a few lines above?
Description was changed from ========== [wasm] Fixed wasm interpreter entry for 32 bit big endian systems The order of the return values were wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, ========== to ========== [wasm] Fix for wasm interpreter entry for 32 bit big endian systems The order of the return values were wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, ==========
Description was changed from ========== [wasm] Fix for wasm interpreter entry for 32 bit big endian systems The order of the return values were wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, ========== to ========== [wasm] Fix for wasm interpreter entry for 32 bit big endian systems The order of the return values are wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, ==========
On 2017/03/21 08:36:10, Clemens Hammacher wrote: > Nit: Please write the commit message in present tense: "Fix ..." > > https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.cc > File src/compiler/wasm-compiler.cc (right): > > https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.... > src/compiler/wasm-compiler.cc:2852: #if defined(V8_TARGET_LITTLE_ENDIAN) > Instead of introducing another "#if defined(...)", can you please use the > kInt64LowerHalfMemoryOffset and kInt64UpperHalfMemoryOffset constants to load > the lower and upper half, similar to the store nodes for the parameters a few > lines above? ptal, using the constants to load the upper and lower half now instead of using the macros
Description was changed from ========== [wasm] Fix for wasm interpreter entry for 32 bit big endian systems The order of the return values are wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, ========== to ========== [wasm] Fix wasm interpreter entry for 32 bit big endian systems The order of the return values are wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, ==========
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/2764583003/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:2852: #if defined(V8_TARGET_LITTLE_ENDIAN) On 2017/03/21 08:36:10, Clemens Hammacher wrote: > Instead of introducing another "#if defined(...)", can you please use the > kInt64LowerHalfMemoryOffset and kInt64UpperHalfMemoryOffset constants to load > the lower and upper half, similar to the store nodes for the parameters a few > lines above? Done.
The CQ bit was unchecked by rayb@ca.ibm.com
On 2017/03/22 at 17:19:52, rayb wrote: > On 2017/03/21 08:36:10, Clemens Hammacher wrote: > > Nit: Please write the commit message in present tense: "Fix ..." > > > > https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.cc > > File src/compiler/wasm-compiler.cc (right): > > > > https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.... > > src/compiler/wasm-compiler.cc:2852: #if defined(V8_TARGET_LITTLE_ENDIAN) > > Instead of introducing another "#if defined(...)", can you please use the > > kInt64LowerHalfMemoryOffset and kInt64UpperHalfMemoryOffset constants to load > > the lower and upper half, similar to the store nodes for the parameters a few > > lines above? > > ptal, using the constants to load the upper and lower half now instead of using the macros LGTM. (I fixed the title)
On 2017/03/22 17:22:01, Clemens Hammacher wrote: > On 2017/03/22 at 17:19:52, rayb wrote: > > On 2017/03/21 08:36:10, Clemens Hammacher wrote: > > > Nit: Please write the commit message in present tense: "Fix ..." > > > > > > > https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.cc > > > File src/compiler/wasm-compiler.cc (right): > > > > > > > https://codereview.chromium.org/2764583003/diff/1/src/compiler/wasm-compiler.... > > > src/compiler/wasm-compiler.cc:2852: #if defined(V8_TARGET_LITTLE_ENDIAN) > > > Instead of introducing another "#if defined(...)", can you please use the > > > kInt64LowerHalfMemoryOffset and kInt64UpperHalfMemoryOffset constants to > load > > > the lower and upper half, similar to the store nodes for the parameters a > few > > > lines above? > > > > ptal, using the constants to load the upper and lower half now instead of > using the macros > > LGTM. > (I fixed the title) Thanks for fixing the title
The CQ bit was checked by rayb@ca.ibm.com
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": 1490204825522360,
"parent_rev": "9377fd1a462a16aa9220df81e5d6c8de988d4a47", "commit_rev":
"04440d28693474cf0a7c10c5a63915528a227a5f"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix wasm interpreter entry for 32 bit big endian systems The order of the return values are wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, ========== to ========== [wasm] Fix wasm interpreter entry for 32 bit big endian systems The order of the return values are wrong for 32 bit big endian machines. BUG=none R=titzer@chromium.org, clemensh@chromium.org, Review-Url: https://codereview.chromium.org/2764583003 Cr-Commit-Position: refs/heads/master@{#44041} Committed: https://chromium.googlesource.com/v8/v8/+/04440d28693474cf0a7c10c5a63915528a2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/04440d28693474cf0a7c10c5a63915528a2... |
