Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843523002:
[wasm] Implement simd lowering for I16x8 (Closed)
Description[wasm] Implement simd lowering for I16x8
R=bbudge@chromium.org,gdeepti@chromium.org,mtrofin@chromium.org
BUG=v8:6020
Review-Url: https://codereview.chromium.org/2843523002
Cr-Commit-Position: refs/heads/master@{#45004}
Committed: https://chromium.googlesource.com/v8/v8/+/cda2e2dd91da6e70505d2e314454d3b66c0bb3a6
Patch Set 1 #
Total comments: 10
Patch Set 2 : [wasm] Implement simd lowering for I16x8 #
Total comments: 6
Patch Set 3 : [wasm] Implement simd lowering for I16x8 #
Total comments: 14
Patch Set 4 : [wasm] Implement simd lowering for I16x8 #
Total comments: 2
Patch Set 5 : [wasm] Implement simd lowering for I16x8 #
Total comments: 2
Patch Set 6 : [wasm] Implement simd lowering for I16x8 #
Depends on Patchset: Messages
Total messages: 29 (13 generated)
The CQ bit was checked by aseemgarg@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.
https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:330: effect_input, control_input); You can move this into the loop if you define another local: Node* last_effect_input = effect_input; Then at the end of your loop, assign the newly created rep_node: last_effect_input = rep_nodes[i]; https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:406: max = std::numeric_limits<int16_t>::max(); I'm not clear why these are hard-coded, when the method looks more general (and there will be saturating Int8x16 operations.) https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:548: node->opcode() == IrOpcode::kI16x8Shl) { There will be more shifts, so perhaps a switch statement might work better here. https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:39: static const int kMaxLanes32 = 4; nit: These seem like numbers of lanes, rather than maximum number of lanes, so 'kNumLanes32'? https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:41: static const int kWidth = 16; You can use kSimd128Size, defined in globals.h.
Description was changed from ========== [wasm] Implement simd lowering for I16x8 R=bbudge@chromium.org,gdeepti@chromium.org,mtrofin@chromium.org BUG=v8:6020 ========== to ========== [wasm] Implement simd lowering for I16x8 R=bbudge@chromium.org,gdeepti@chromium.org,mtrofin@chromium.org BUG=v8:6020 ==========
aseemgarg@chromium.org changed reviewers: + bradnelson@chromium.org
https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:330: effect_input, control_input); On 2017/04/24 20:25:04, bbudge wrote: > You can move this into the loop if you define another local: > > Node* last_effect_input = effect_input; > > Then at the end of your loop, assign the newly created rep_node: > > last_effect_input = rep_nodes[i]; Done. https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:406: max = std::numeric_limits<int16_t>::max(); On 2017/04/24 20:25:04, bbudge wrote: > I'm not clear why these are hard-coded, when the method looks more general (and > there will be saturating Int8x16 operations.) This will change when Int8x16 comes in. https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:548: node->opcode() == IrOpcode::kI16x8Shl) { On 2017/04/24 20:25:04, bbudge wrote: > There will be more shifts, so perhaps a switch statement might work better here. Done. https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:39: static const int kMaxLanes32 = 4; On 2017/04/24 20:25:04, bbudge wrote: > nit: These seem like numbers of lanes, rather than maximum number of lanes, so > 'kNumLanes32'? Done. https://codereview.chromium.org/2843523002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:41: static const int kWidth = 16; On 2017/04/24 20:25:04, bbudge wrote: > You can use kSimd128Size, defined in globals.h. Done.
https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:85: V(I32x4ShrU) \ Reorder so shifts are with other Unary ops to be consistent with the arch implementations and 16x8 ops below? https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:638: load_op = machine()->Load(MachineType::Int16()); Might be useful here to have a macro with SimdType and MachineType defined to reduce duplication for all the laod, store ops here which can also be used for the Phi replacement function at the end of the file. It will also make adding I8x16 opcodes for these easier i.e. just an addition to the macro. https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:36: kSimd1x8 Perhaps maintain the same ordering for ops as in instruction-selector/wasm-opcodes etc.? (F32x4, I32x4, I16x8, I8x16, Generic S128 types).
https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:85: V(I32x4ShrU) \ On 2017/04/25 20:29:57, gdeepti wrote: > Reorder so shifts are with other Unary ops to be consistent with the arch > implementations and 16x8 ops below? Done. https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:638: load_op = machine()->Load(MachineType::Int16()); On 2017/04/25 20:29:57, gdeepti wrote: > Might be useful here to have a macro with SimdType and MachineType defined to > reduce duplication for all the laod, store ops here which can also be used for > the Phi replacement function at the end of the file. It will also make adding > I8x16 opcodes for these easier i.e. just an addition to the macro. Done. https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/20001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:36: kSimd1x8 On 2017/04/25 20:29:58, gdeepti wrote: > Perhaps maintain the same ordering for ops as in > instruction-selector/wasm-opcodes etc.? (F32x4, I32x4, I16x8, I8x16, Generic > S128 types). Done.
https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:553: rep_node[i] = Mask(rep_node[i], kMask16); Comment that you intend to fall through here, i.e. // fall through https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:571: if (node->opcode() == IrOpcode::kI16x8Shl) { Move this up with the shift cases it modifies. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:1023: void SimdScalarLowering::ReplaceNode(Node* old, Node** new_node, int count) { nit: new_nodes, to clarify it's an array. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:1091: Int16ToInt32(replacements, result); It seems strange to call these int conversions if they're unimplemented. Maybe leave them off the patch and add them later? https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:35: kSimd1x4, It's strange to see lane types (e.g. kFloat32) mixed with SIMD types here. I think you could eliminate the boolean vector types, and just use the lane type, so instead of kSimd1x4 it could be kInt32. I'm assuming you're treating boolean vectors as a bunch of integers, and not as bit masks. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:37: }; On further thought, is this enum really needed? Could you use MachineRepresentation? I don't see any need to distinguish the boolean vector types. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:42: static const int32_t kShift16 = 16; Define these in the .cc file in an anonymous namespace.
https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:553: rep_node[i] = Mask(rep_node[i], kMask16); On 2017/04/27 18:03:42, bbudge wrote: > Comment that you intend to fall through here, i.e. // fall through Done. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:571: if (node->opcode() == IrOpcode::kI16x8Shl) { On 2017/04/27 18:03:42, bbudge wrote: > Move this up with the shift cases it modifies. removed if and separated cases. Seems cleaner. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:1023: void SimdScalarLowering::ReplaceNode(Node* old, Node** new_node, int count) { On 2017/04/27 18:03:42, bbudge wrote: > nit: new_nodes, to clarify it's an array. Done. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:1091: Int16ToInt32(replacements, result); On 2017/04/27 18:03:42, bbudge wrote: > It seems strange to call these int conversions if they're unimplemented. Maybe > leave them off the patch and add them later? Done. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:35: kSimd1x4, On 2017/04/27 18:03:42, bbudge wrote: > It's strange to see lane types (e.g. kFloat32) mixed with SIMD types here. I > think you could eliminate the boolean vector types, and just use the lane type, > so instead of kSimd1x4 it could be kInt32. I'm assuming you're treating boolean > vectors as a bunch of integers, and not as bit masks. We do need the various types. Basically, this is to ensure that Int32x4 is not mixed with Simd1x4 (even though boolean types are implemented using int32). But I guess the naming would be better if I used kInt32x4 instead of kInt32 and so on. Made the change. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:37: }; On 2017/04/27 18:03:42, bbudge wrote: > On further thought, is this enum really needed? Could you use > MachineRepresentation? I don't see any need to distinguish the boolean vector > types. Machine representation won't work as we need to distinguish between the various Simd128 types. https://codereview.chromium.org/2843523002/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:42: static const int32_t kShift16 = 16; On 2017/04/27 18:03:42, bbudge wrote: > Define these in the .cc file in an anonymous namespace. Done.
lgtm https://codereview.chromium.org/2843523002/diff/60001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/60001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:560: rep_node[i] = Mask(rep_node[i], kMask16); // Fallthrough nit: Fall through.
https://codereview.chromium.org/2843523002/diff/60001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2843523002/diff/60001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:560: rep_node[i] = Mask(rep_node[i], kMask16); // Fallthrough On 2017/05/01 18:42:27, bbudge wrote: > nit: Fall through. Done.
mtrofin@google.com changed reviewers: + mtrofin@google.com
lgtm, 1 nit https://codereview.chromium.org/2843523002/diff/80001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/80001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:40: Node** node; please init this field and the one below.
https://codereview.chromium.org/2843523002/diff/80001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2843523002/diff/80001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.h:40: Node** node; On 2017/05/01 19:54:24, mtrofin wrote: > please init this field and the one below. Done.
The CQ bit was checked by aseemgarg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org, mtrofin@google.com Link to the patchset: https://codereview.chromium.org/2843523002/#ps100001 (title: "[wasm] Implement simd lowering for I16x8")
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/40201)
On 2017/05/01 21:01:59, commit-bot: I haz the power wrote: > 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/40201) lgtm
The CQ bit was checked by aseemgarg@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": 100001, "attempt_start_ts": 1493673385422670,
"parent_rev": "f79c3b51426b5b68f044593979a674106a5432c6", "commit_rev":
"cda2e2dd91da6e70505d2e314454d3b66c0bb3a6"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Implement simd lowering for I16x8 R=bbudge@chromium.org,gdeepti@chromium.org,mtrofin@chromium.org BUG=v8:6020 ========== to ========== [wasm] Implement simd lowering for I16x8 R=bbudge@chromium.org,gdeepti@chromium.org,mtrofin@chromium.org BUG=v8:6020 Review-Url: https://codereview.chromium.org/2843523002 Cr-Commit-Position: refs/heads/master@{#45004} Committed: https://chromium.googlesource.com/v8/v8/+/cda2e2dd91da6e70505d2e314454d3b66c0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/cda2e2dd91da6e70505d2e314454d3b66c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
