|
|
Created:
3 years, 9 months ago by gdeepti Modified:
3 years, 8 months ago Reviewers:
Benedikt Meurer, bbudge, Mircea Trofin, aseemgarg CC:
v8-reviews_googlegroups.com, v8-mips-ports_googlegroups.com, v8-ppc-ports_googlegroups.com, wasm-v8_google.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Make Opcode names consistent across architectures, implementations
- Fix opcode names to be consistent with opcodes as in wasm-opcodes.h
- Fix Ordering of Ops, inconsistencies
BUG=v8:6020
Review-Url: https://codereview.chromium.org/2776753004
Cr-Commit-Position: refs/heads/master@{#44239}
Committed: https://chromium.googlesource.com/v8/v8/+/6234fda3c9dcf223a243172d03ea1588f9cfe28d
Patch Set 1 #Patch Set 2 : Fix Float32x4 -> F32x4 #Patch Set 3 : Fix left over F32x4 case #Patch Set 4 : Fix Int32x4 types #Patch Set 5 : Fix Int32x4 Shifts #Patch Set 6 : Fix shifts #Patch Set 7 : Fix Int16x8, Int8x16 types #Patch Set 8 : Fix generic S128 types #Patch Set 9 : Fix swizzles #Patch Set 10 : Fix Arm build #Patch Set 11 : Fix Ordering #Patch Set 12 : Scalar fix #
Total comments: 11
Patch Set 13 : Bill's review #Patch Set 14 : Fix build #Patch Set 15 : Fix convert order #
Total comments: 2
Patch Set 16 : Fix missing Convert #
Total comments: 2
Patch Set 17 : Fix Saturates #
Created: 3 years, 8 months ago
Messages
Total messages: 98 (83 generated)
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23282) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19771)
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23283) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19772)
The CQ bit was checked by gdeepti@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...
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 gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/23330) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19792)
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23304) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/23221) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19793)
The CQ bit was checked by gdeepti@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...
The CQ bit was checked by gdeepti@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...
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 gdeepti@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...
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 gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19854)
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
The CQ bit was checked by gdeepti@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...
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 gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23376) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19868)
The CQ bit was checked by gdeepti@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 ========== [wasm] Make Opcode names consistent across architectures, implementations BUG=v8:6020 ========== to ========== [wasm] Make Opcode names consistent across architectures, implementations - Fix opcode names to be consistent with opcodes as in wasm-opcodes.h - Fix Ordering of Ops, inconsistencies BUG=v8:6020 ==========
gdeepti@chromium.org changed reviewers: + aseemgarg@chromium.org, bbudge@chromium.org, mtrofin@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I will be taking a second look to make sure the ordering is consistent, sending out for pre-review as the CL has quite a bit of churn. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/machine-o... File src/compiler/machine-operator.h (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/machine-o... src/compiler/machine-operator.h:541: const Operator* Simd128Store3(); Can these be removed? Or are we still considering these as future SIMD ops?
https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/code-... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/code-... src/compiler/arm/code-generator-arm.cc:1645: case kArmF32x4SConvertI32x4: { these are unary ops, so should remain above. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/code-... src/compiler/arm/code-generator-arm.cc:1696: case kArmI32x4Shl: { Shifts are unary and should be moved back. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/instr... File src/compiler/arm/instruction-codes-arm.h (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/instr... src/compiler/arm/instruction-codes-arm.h:222: V(ArmSimd128Select) \ Simd -> S https://codereview.chromium.org/2776753004/diff/220001/src/compiler/machine-o... File src/compiler/machine-operator.h (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/machine-o... src/compiler/machine-operator.h:541: const Operator* Simd128Store3(); On 2017/03/27 23:02:30, gdeepti wrote: > Can these be removed? Or are we still considering these as future SIMD ops? Go ahead and remove them. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/opcodes.h File src/compiler/opcodes.h (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/opcodes.h... src/compiler/opcodes.h:672: V(Simd128Load3) \ Remove the partial loads/stores
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/23451) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by gdeepti@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...
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 gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
The CQ bit was checked by gdeepti@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/2776753004/diff/220001/src/compiler/arm/code-... File src/compiler/arm/code-generator-arm.cc (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/code-... src/compiler/arm/code-generator-arm.cc:1645: case kArmF32x4SConvertI32x4: { On 2017/03/28 00:13:26, bbudge wrote: > these are unary ops, so should remain above. Done. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/code-... src/compiler/arm/code-generator-arm.cc:1696: case kArmI32x4Shl: { On 2017/03/28 00:13:26, bbudge wrote: > Shifts are unary and should be moved back. Done. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/instr... File src/compiler/arm/instruction-codes-arm.h (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/arm/instr... src/compiler/arm/instruction-codes-arm.h:222: V(ArmSimd128Select) \ On 2017/03/28 00:13:26, bbudge wrote: > Simd -> S Done. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/machine-o... File src/compiler/machine-operator.h (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/machine-o... src/compiler/machine-operator.h:541: const Operator* Simd128Store3(); On 2017/03/28 00:13:26, bbudge wrote: > On 2017/03/27 23:02:30, gdeepti wrote: > > Can these be removed? Or are we still considering these as future SIMD ops? > > Go ahead and remove them. Done. https://codereview.chromium.org/2776753004/diff/220001/src/compiler/opcodes.h File src/compiler/opcodes.h (right): https://codereview.chromium.org/2776753004/diff/220001/src/compiler/opcodes.h... src/compiler/opcodes.h:672: V(Simd128Load3) \ On 2017/03/28 00:13:26, bbudge wrote: > Remove the partial loads/stores Done.
https://codereview.chromium.org/2776753004/diff/280001/src/compiler/arm/code-... File src/compiler/arm/code-generator-arm.cc (left): https://codereview.chromium.org/2776753004/diff/280001/src/compiler/arm/code-... src/compiler/arm/code-generator-arm.cc:1573: case kArmFloat32x4FromInt32x4: { seems like you forgot to create the convert cases for these two.
https://codereview.chromium.org/2776753004/diff/280001/src/compiler/arm/code-... File src/compiler/arm/code-generator-arm.cc (left): https://codereview.chromium.org/2776753004/diff/280001/src/compiler/arm/code-... src/compiler/arm/code-generator-arm.cc:1573: case kArmFloat32x4FromInt32x4: { On 2017/03/28 22:25:22, aseemgarg wrote: > seems like you forgot to create the convert cases for these two. Fixed in the patch I uploaded right after this one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm https://codereview.chromium.org/2776753004/diff/300001/src/compiler/arm/instr... File src/compiler/arm/instruction-scheduler-arm.cc (right): https://codereview.chromium.org/2776753004/diff/300001/src/compiler/arm/instr... src/compiler/arm/instruction-scheduler-arm.cc:136: case kArmI32x4UConvertF32x4: nit: out of order
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed inconsistency with Signed AddSaturate/SubSaturate to AddSaturateS/SubSaturateS. https://codereview.chromium.org/2776753004/diff/300001/src/compiler/arm/instr... File src/compiler/arm/instruction-scheduler-arm.cc (right): https://codereview.chromium.org/2776753004/diff/300001/src/compiler/arm/instr... src/compiler/arm/instruction-scheduler-arm.cc:136: case kArmI32x4UConvertF32x4: On 2017/03/29 00:33:19, bbudge wrote: > nit: out of order Done.
gdeepti@chromium.org changed reviewers: + bmeurer@chromium.org
+bmeurer@ for owners in src/compiler.
The CQ bit was checked by gdeepti@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...
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 gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, aseemgarg@chromium.org Link to the patchset: https://codereview.chromium.org/2776753004/#ps320001 (title: "Fix Saturates")
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
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/37905)
rubber stamp LGTM for wasm OWNERS part.
The CQ bit was checked by gdeepti@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": 320001, "attempt_start_ts": 1490806836431680, "parent_rev": "88f71126a5c067f98c75044bc26778f2e8ea2e79", "commit_rev": "6234fda3c9dcf223a243172d03ea1588f9cfe28d"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Make Opcode names consistent across architectures, implementations - Fix opcode names to be consistent with opcodes as in wasm-opcodes.h - Fix Ordering of Ops, inconsistencies BUG=v8:6020 ========== to ========== [wasm] Make Opcode names consistent across architectures, implementations - Fix opcode names to be consistent with opcodes as in wasm-opcodes.h - Fix Ordering of Ops, inconsistencies BUG=v8:6020 Review-Url: https://codereview.chromium.org/2776753004 Cr-Commit-Position: refs/heads/master@{#44239} Committed: https://chromium.googlesource.com/v8/v8/+/6234fda3c9dcf223a243172d03ea1588f9c... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/v8/v8/+/6234fda3c9dcf223a243172d03ea1588f9c... |