Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 3 months ago by aseemgarg Modified:
4 years, 1 month ago Reviewers:
gdeepti, titzer, bradnelson 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[wasm] simd scalar lowering F32x4Add and I32x4Add
BUG=v8:4124
TEST:test-run-wasm-simd-lowering
R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org
Committed: https://crrev.com/cf9ee0ec6c3768bf7f47363056169ce0624348ef
Cr-Commit-Position: refs/heads/master@{#40448}
Patch Set 1 #
Total comments: 32
Patch Set 2 : [wasm] simd scalar lowering F32x4Add and I32x4Add #Patch Set 3 : [wasm] simd scalar lowering F32x4Add and I32x4Add #
Total comments: 14
Patch Set 4 : [wasm] simd scalar lowering F32x4Add and I32x4Add #
Total comments: 2
Patch Set 5 : [wasm] simd scalar lowering F32x4Add and I32x4Add #
Created: 4 years, 2 months ago
Depends on Patchset: Messages
Total messages: 31 (18 generated)
Please see if basic idea looks fine. Will submit after finishing with tests for function calls. Still need to deal with extract lane properly..
https://codereview.chromium.org/2294743003/diff/1/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:222: 'wasm/test-run-wasm-simd-lowering.cc', Is there a reason we run the tests only on x64? Ignore if you're using x64 as a starting point I'm just wondering if you hit anything that was architecture specific. https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-simd-lowering.cc:34: TEST(Simd_I32x4_Splat) { WASM_EXEC_TEST here could work here too if you want less overhead. A scalar lowering pass for testing in wasm-run-utils.h might need to be added for the tests to work correctly. (See Int64LoweringForTesting())
https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:78: SetLoweredType ? https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:89: FOREACH_FLOAT32X4_OPCODE(CASE_STMT) { Indent is weird, did this make it through git cl presubmit? https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:104: result += 3; Comment explaining this is single items going to 4. (Maybe compute from a constant for lane count) I'm currently assuming that for 8x16 + 16x8 we'll use 4 ints and introduce intemediates as needed to do shifting tricks, so this is probably fine. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:155: Node* new_node[4]; constant https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:223: case IrOpcode::kInt32x4Add: { Macros might be in order once you have more of these, but not for now. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:297: if (HasReplacementHigh(input)) { This naming from the 64-bit stuff is goofy. HasReplacement*(...) -> HasReplacement(index, ...) https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:35: Node* node[4]; 4 -> kMaxLanes? https://codereview.chromium.org/2294743003/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:3114: SimdScalarLowering r(graph, machine, common, jsgraph_->zone(), -> SimdScalarLowering(graph, machine, common, jsgraph_->zone(), function_->sig).LowerGraph(); and maybe above too. https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-module-helper.h (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-module-helper.h:23: class TestRunWasmModuleHelper { Land as a separate refactor (but I think you said you were going to?) https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-simd-lowering.cc:109: SimpleTest(code, sizeof(code), 1); I assume you'll want tests with loops + conditions to cover the phis with cycle case. You're avoiding that for now for lack of get_local / set_local ?
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 aseemgarg@chromium.org
https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:155: Node* new_node[4]; On 2016/08/31 22:42:08, bradnelson wrote: > constant Yes, please use kMaxLanes or similar. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:341: for (int i = 0; i < 4; i++) { No need to initialize these to null if you are going to overwrite them. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:344: if (ReplacementType(node) == SimdType::kInt32 && type == SimdType::kFloat32) { If you get rid of the kUndefined type, this will boil down to just two cases, so no need for the UNREACHABLE case below. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:32: enum class SimdType : uint8_t { kUndefined, kInt32, kFloat32 }; I don't see where you are using the kUndefined here.
https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-module-helper.h (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-module-helper.h:23: class TestRunWasmModuleHelper { On 2016/08/31 22:42:08, bradnelson wrote: > Land as a separate refactor (but I think you said you were going to?) I'm not sure we need to extract this out. It shouldn't be necessary to use the WasmModuleBuilder just to test calls. See for example how test-run-wasm-64.cc works.
https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:78: On 2016/08/31 22:42:08, bradnelson wrote: > SetLoweredType ? Done. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:89: FOREACH_FLOAT32X4_OPCODE(CASE_STMT) { On 2016/08/31 22:42:08, bradnelson wrote: > Indent is weird, did this make it through git cl presubmit? Did git cl format. And the changes seem to have made it through presubmit checks. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:104: result += 3; On 2016/08/31 22:42:08, bradnelson wrote: > Comment explaining this is single items going to 4. (Maybe compute from a > constant for lane count) > I'm currently assuming that for 8x16 + 16x8 we'll use 4 ints and introduce > intemediates as needed to do shifting tricks, so this is probably fine. Done. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:155: Node* new_node[4]; On 2016/09/05 12:59:35, titzer wrote: > On 2016/08/31 22:42:08, bradnelson wrote: > > constant > > Yes, please use kMaxLanes or similar. Done. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:223: case IrOpcode::kInt32x4Add: { On 2016/08/31 22:42:08, bradnelson wrote: > Macros might be in order once you have more of these, but not for now. that's the plan. Macros or functions. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:297: if (HasReplacementHigh(input)) { On 2016/08/31 22:42:08, bradnelson wrote: > This naming from the 64-bit stuff is goofy. > HasReplacement*(...) -> HasReplacement(index, ...) Done. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:341: for (int i = 0; i < 4; i++) { On 2016/09/05 12:59:34, titzer wrote: > No need to initialize these to null if you are going to overwrite them. Actually we do need to mark some null in case there is no replacement for it. But taken care of it differently to make it more clear. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.cc:344: if (ReplacementType(node) == SimdType::kInt32 && type == SimdType::kFloat32) { On 2016/09/05 12:59:34, titzer wrote: > If you get rid of the kUndefined type, this will boil down to just two cases, so > no need for the UNREACHABLE case below. Done. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:32: enum class SimdType : uint8_t { kUndefined, kInt32, kFloat32 }; On 2016/09/05 12:59:35, titzer wrote: > I don't see where you are using the kUndefined here. Done. https://codereview.chromium.org/2294743003/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:35: Node* node[4]; On 2016/08/31 22:42:08, bradnelson wrote: > 4 -> kMaxLanes? Done. https://codereview.chromium.org/2294743003/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2294743003/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:3114: SimdScalarLowering r(graph, machine, common, jsgraph_->zone(), On 2016/08/31 22:42:08, bradnelson wrote: > -> > SimdScalarLowering(graph, machine, common, jsgraph_->zone(), > function_->sig).LowerGraph(); > > and maybe above too. Done. https://codereview.chromium.org/2294743003/diff/1/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:222: 'wasm/test-run-wasm-simd-lowering.cc', On 2016/08/30 17:16:06, gdeepti wrote: > Is there a reason we run the tests only on x64? Ignore if you're using x64 as a > starting point I'm just wondering if you hit anything that was architecture > specific. I set it up to run on architectures where the other simd tests are running. https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-module-helper.h (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-module-helper.h:23: class TestRunWasmModuleHelper { On 2016/08/31 22:42:08, bradnelson wrote: > Land as a separate refactor (but I think you said you were going to?) removed file. Moved to different test type. https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-simd-lowering.cc:34: TEST(Simd_I32x4_Splat) { On 2016/08/30 17:16:06, gdeepti wrote: > WASM_EXEC_TEST here could work here too if you want less overhead. A scalar > lowering pass for testing in wasm-run-utils.h might need to be added for the > tests to work correctly. (See Int64LoweringForTesting()) Done. https://codereview.chromium.org/2294743003/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-simd-lowering.cc:109: SimpleTest(code, sizeof(code), 1); On 2016/08/31 22:42:08, bradnelson wrote: > I assume you'll want tests with loops + conditions to cover the phis with cycle > case. > You're avoiding that for now for lack of get_local / set_local ? yes. Will do in next change.
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: Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/15861)
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.
Looks mostly good https://codereview.chromium.org/2294743003/diff/40001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:135: int parameter_count = GetParameterCountAfterLowering(signature()); We should probably cache the GetParameterCountAfterLowering(signature()) value as a field. https://codereview.chromium.org/2294743003/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:153: int old_index = ParameterIndexOf(node->op()); Probably want to skip all of this if old_index == new_index (and not SIMD128). https://codereview.chromium.org/2294743003/diff/40001/src/compiler/wasm-linka... File src/compiler/wasm-linkage.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/compiler/wasm-linka... src/compiler/wasm-linkage.cc:310: CallDescriptor* GetI32WasmCallDescriptorWithReplacement( The name is a bit weird now, since it is no longer I32-specific. This basically takes a call descriptor and replaces all occurences of type {in} with N occurrences of type {out}. ReplaceTypeInCallDescriptorWith() ? https://codereview.chromium.org/2294743003/diff/40001/src/compiler/wasm-linka... src/compiler/wasm-linkage.cc:350: locations.AddParam(params.Next(MachineRepresentation::kWord32)); kWord32 should be a parameter to this method now. https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:244: WasmOpcode opcode = ExtractWasmOpcode(pc); Could you do the trick in the main decoder loop of making the SIMD prefix one of the cases here, so we save a branch in the common case? https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:1261: unsigned ExtractLane(WasmOpcode opcode, LocalType type) { Please make a LaneOperand class similar to the others in wasm-opcodes.h I think you can keep it in the .cc file for now, though.
https://codereview.chromium.org/2294743003/diff/40001/src/compiler/simd-scala... File src/compiler/simd-scalar-lowering.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:135: int parameter_count = GetParameterCountAfterLowering(signature()); On 2016/10/12 19:06:55, titzer wrote: > We should probably cache the GetParameterCountAfterLowering(signature()) value > as a field. Done. https://codereview.chromium.org/2294743003/diff/40001/src/compiler/simd-scala... src/compiler/simd-scalar-lowering.cc:153: int old_index = ParameterIndexOf(node->op()); On 2016/10/12 19:06:55, titzer wrote: > Probably want to skip all of this if old_index == new_index (and not SIMD128). Done. https://codereview.chromium.org/2294743003/diff/40001/src/compiler/wasm-linka... File src/compiler/wasm-linkage.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/compiler/wasm-linka... src/compiler/wasm-linkage.cc:310: CallDescriptor* GetI32WasmCallDescriptorWithReplacement( On 2016/10/12 19:06:55, titzer wrote: > The name is a bit weird now, since it is no longer I32-specific. > > This basically takes a call descriptor and replaces all occurences of type {in} > with N occurrences of type {out}. > > ReplaceTypeInCallDescriptorWith() ? Done. https://codereview.chromium.org/2294743003/diff/40001/src/compiler/wasm-linka... src/compiler/wasm-linkage.cc:350: locations.AddParam(params.Next(MachineRepresentation::kWord32)); On 2016/10/12 19:06:55, titzer wrote: > kWord32 should be a parameter to this method now. Done. https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:244: WasmOpcode opcode = ExtractWasmOpcode(pc); On 2016/10/12 19:06:55, titzer wrote: > Could you do the trick in the main decoder loop of making the SIMD prefix one of > the cases here, so we save a branch in the common case? Done. Actually seems like we don't need this at all. https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:1261: unsigned ExtractLane(WasmOpcode opcode, LocalType type) { On 2016/10/12 19:06:55, titzer wrote: > Please make a LaneOperand class similar to the others in wasm-opcodes.h > > I think you can keep it in the .cc file for now, though. Done.
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.
lgtm modulo last comment https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:244: WasmOpcode opcode = ExtractWasmOpcode(pc); On 2016/10/17 19:01:44, aseemgarg wrote: > On 2016/10/12 19:06:55, titzer wrote: > > Could you do the trick in the main decoder loop of making the SIMD prefix one > of > > the cases here, so we save a branch in the common case? > > Done. Actually seems like we don't need this at all. No, you do, since we still want to be able to iterate over SIMD bytecodes, which are not a single byte. https://codereview.chromium.org/2294743003/diff/60001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2294743003/diff/60001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:1277: len = ExtractLane(opcode, LocalType::kWord32); Can you follow the pattern where the LaneOperand is instantiated and there is a Validate() method that returns a boolean?
https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2294743003/diff/40001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:244: WasmOpcode opcode = ExtractWasmOpcode(pc); On 2016/10/19 18:31:49, titzer wrote: > On 2016/10/17 19:01:44, aseemgarg wrote: > > On 2016/10/12 19:06:55, titzer wrote: > > > Could you do the trick in the main decoder loop of making the SIMD prefix > one > > of > > > the cases here, so we save a branch in the common case? > > > > Done. Actually seems like we don't need this at all. > > No, you do, since we still want to be able to iterate over SIMD bytecodes, which > are not a single byte. Done. https://codereview.chromium.org/2294743003/diff/60001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2294743003/diff/60001/src/wasm/ast-decoder.cc... src/wasm/ast-decoder.cc:1277: len = ExtractLane(opcode, LocalType::kWord32); On 2016/10/19 18:31:49, titzer wrote: > Can you follow the pattern where the LaneOperand is instantiated and there is a > Validate() method that returns a boolean? Done.
The CQ bit was checked by aseemgarg@chromium.org
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/2294743003/#ps80001 (title: "[wasm] simd scalar lowering F32x4Add and I32x4Add")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] simd scalar lowering F32x4Add and I32x4Add BUG=v8:4124 TEST:test-run-wasm-simd-lowering R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org ========== to ========== [wasm] simd scalar lowering F32x4Add and I32x4Add BUG=v8:4124 TEST:test-run-wasm-simd-lowering R=titzer@chromium.org,bradnelson@chromium.org,gdeepti@chromium.org Committed: https://crrev.com/cf9ee0ec6c3768bf7f47363056169ce0624348ef Cr-Commit-Position: refs/heads/master@{#40448} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cf9ee0ec6c3768bf7f47363056169ce0624348ef Cr-Commit-Position: refs/heads/master@{#40448} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
