Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514663002:
[simd] Disable Simd Scalar lowering for x64, enable tests for all other architectures. (Closed)
Description[simd] Disable Simd Scalar lowering for x64, enable tests for all other architectures.
- Simd Scalar lowering should be conditionally disabled if the architecture has a native SIMD implementation.
- Enable scalar lowering tests on all architectures instead of only x64.
R=bbudge@chromium.org, aseemgarg@chromium.org
Committed: https://crrev.com/e60e961140a1cae0c91ee0d7141c765bc193ab6c
Cr-Commit-Position: refs/heads/master@{#41160}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Change build files #Patch Set 3 : Bill's review #
Total comments: 2
Patch Set 4 : Bill's review #
Messages
Total messages: 32 (19 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2514663002/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2514663002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:56: bool IsScalarLoweringEnabled() { How hard is it to just not even create a SimdScalarLowering phase on platforms that support SIMD natively? I think that would be cleaner than creating this object and then silently doing nothing when the lower function is called. Also, can you use the Assembler::SupportsSimd128 function to determine whether lowering is necessary? https://cs.chromium.org/chromium/src/v8/src/x64/assembler-x64-inl.h?rcl=14792... https://codereview.chromium.org/2514663002/diff/1/test/cctest/BUILD.gn File test/cctest/BUILD.gn (left): https://codereview.chromium.org/2514663002/diff/1/test/cctest/BUILD.gn#oldcod... test/cctest/BUILD.gn:282: ] You can remove the lowering tests from sources here, i.e. sources -= [ "wasm/test-run-wasm-simd-lowering.cc" ] Then you don't have to comment out the tests for archs that support SIMD without lowering. Alternatively we could only add this file to sources for platforms that don't support SIMD, which is all of them except x64 right now. That might be the clearest, although it's more verbose. https://codereview.chromium.org/2514663002/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right): https://codereview.chromium.org/2514663002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-simd-lowering.cc:18: #if !V8_TARGET_ARCH_X64 Rather than comment out the code here, don't include these tests in the build files.
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...
https://codereview.chromium.org/2514663002/diff/1/src/compiler/simd-scalar-lo... File src/compiler/simd-scalar-lowering.h (right): https://codereview.chromium.org/2514663002/diff/1/src/compiler/simd-scalar-lo... src/compiler/simd-scalar-lowering.h:56: bool IsScalarLoweringEnabled() { On 2016/11/18 19:30:59, bbudge wrote: > How hard is it to just not even create a SimdScalarLowering phase on platforms > that support SIMD natively? I think that would be cleaner than creating this > object and then silently doing nothing when the lower function is called. > > Also, can you use the Assembler::SupportsSimd128 function to determine whether > lowering is necessary? > > https://cs.chromium.org/chromium/src/v8/src/x64/assembler-x64-inl.h?rcl=14792... Fixed to use SupportsSimd128 function, moved conditional to wasm-compiler.cc so object is not created. https://codereview.chromium.org/2514663002/diff/1/test/cctest/BUILD.gn File test/cctest/BUILD.gn (left): https://codereview.chromium.org/2514663002/diff/1/test/cctest/BUILD.gn#oldcod... test/cctest/BUILD.gn:282: ] On 2016/11/18 19:30:59, bbudge wrote: > You can remove the lowering tests from sources here, i.e. > sources -= [ > "wasm/test-run-wasm-simd-lowering.cc" > ] > > Then you don't have to comment out the tests for archs that support SIMD without > lowering. > > Alternatively we could only add this file to sources for platforms that don't > support SIMD, which is all of them except x64 right now. That might be the > clearest, although it's more verbose. I was not sure which way was cleaner as most architectures currently do not support SIMD, and this particular test file had no architecture specific functionality. Fixed to add to sources only for platforms that do not support SIMD. https://codereview.chromium.org/2514663002/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-simd-lowering.cc (right): https://codereview.chromium.org/2514663002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-simd-lowering.cc:18: #if !V8_TARGET_ARCH_X64 On 2016/11/18 19:30:59, bbudge wrote: > Rather than comment out the code here, don't include these tests in the build > files. Done.
I like the way you changed the build files. I think that's the cleanest and easiest for future addition of native SIMD implementations. LGTM w/1 comment https://codereview.chromium.org/2514663002/diff/40001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2514663002/diff/40001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:3083: if (!CpuFeatures::SupportsSimd128()) { Can you move the test up into TestGraphBuilder? https://cs.chromium.org/chromium/src/v8/test/cctest/wasm/wasm-run-utils.h?rcl...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2514663002/diff/40001/src/compiler/wasm-compi... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2514663002/diff/40001/src/compiler/wasm-compi... src/compiler/wasm-compiler.cc:3083: if (!CpuFeatures::SupportsSimd128()) { On 2016/11/21 19:48:09, bbudge wrote: > Can you move the test up into TestGraphBuilder? > > https://cs.chromium.org/chromium/src/v8/test/cctest/wasm/wasm-run-utils.h?rcl... Done.
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 Link to the patchset: https://codereview.chromium.org/2514663002/#ps60001 (title: "Bill's review")
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/29043)
gdeepti@chromium.org changed reviewers: + mtrofin@chromium.org
Adding mtrofin@ for owners review for wasm-compiler.cc
Adding mtrofin@ for owners review for wasm-compiler.cc
lgtm
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": 60001, "attempt_start_ts": 1479769250117890,
"parent_rev": "d886d8c9b823c877d6c4b8537218beea88907e1e", "commit_rev":
"f70cd517ef4b18d45d5169014a18e6b5dff88366"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [simd] Disable Simd Scalar lowering for x64, enable tests for all other architectures. - Simd Scalar lowering should be conditionally disabled if the architecture has a native SIMD implementation. - Enable scalar lowering tests on all architectures instead of only x64. R=bbudge@chromium.org, aseemgarg@chromium.org ========== to ========== [simd] Disable Simd Scalar lowering for x64, enable tests for all other architectures. - Simd Scalar lowering should be conditionally disabled if the architecture has a native SIMD implementation. - Enable scalar lowering tests on all architectures instead of only x64. R=bbudge@chromium.org, aseemgarg@chromium.org Committed: https://crrev.com/e60e961140a1cae0c91ee0d7141c765bc193ab6c Cr-Commit-Position: refs/heads/master@{#41160} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e60e961140a1cae0c91ee0d7141c765bc193ab6c Cr-Commit-Position: refs/heads/master@{#41160} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
