Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1302133002:
[simd.js] Add SIMD load functions for Phase 1. (Closed)
Description[simd.js] Add SIMD load functions for Phase 1.
Float32x4, Int32x4, Uint32x4:
load, load1, load2, load3
Int16x8, Int8x16, Uint16x8, Uint8x16:
load
BUG=v8:4124
LOG=N
Committed: https://crrev.com/a6754d8c3c6541ac2f35cf151f963bdf1fe595ad
Cr-Commit-Position: refs/heads/master@{#30422}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Bill's comments #
Total comments: 6
Patch Set 3 : Bill's comments #
Total comments: 2
Patch Set 4 : Bill's comment #
Total comments: 4
Patch Set 5 : Rebase, adding unsigned load functions, review comments. #Patch Set 6 : Fixed stale build, verified tests with polyfill disabled #Patch Set 7 : Fixed format #
Messages
Total messages: 46 (16 generated)
gdeepti@chromium.org changed reviewers: + bbudge@chromium.org, dehrenberg@chromium.org, jarin@chromium.org
https://codereview.chromium.org/1302133002/diff/1/src/harmony-simd.js File src/harmony-simd.js (right): https://codereview.chromium.org/1302133002/diff/1/src/harmony-simd.js#newcode339 src/harmony-simd.js:339: } You should be able to use the V8 macro system on these to reduce duplication. There are examples in this file, above. https://codereview.chromium.org/1302133002/diff/1/src/messages.h File src/messages.h (right): https://codereview.chromium.org/1302133002/diff/1/src/messages.h#newcode268 src/messages.h:268: T(InvalidTypedArrayIndex, "The value of Index is invalid") \ You should add a test in test/mjsunit/messages.js (search for 'simd' for an example.) I noticed there is no test for the message on line above though. https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime-simd.cc File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime-simd.cc... src/runtime/runtime-simd.cc:830: FUNCTION(Int8x16, int8_t, 16) You'll have to partition these, since only the 4-lane types have LoadN / StoreN. https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime.h File src/runtime/runtime.h (right): https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime.h#newco... src/runtime/runtime.h:693: F(Int16x8Load3, 2, 1) \ The spec no longer defines the LoadN / StoreN functions on types with 8 or 16 lanes, so you can remove them here and below.
https://codereview.chromium.org/1302133002/diff/1/src/harmony-simd.js File src/harmony-simd.js (right): https://codereview.chromium.org/1302133002/diff/1/src/harmony-simd.js#newcode339 src/harmony-simd.js:339: } On 2015/08/20 18:23:34, bbudge wrote: > You should be able to use the V8 macro system on these to reduce duplication. > There are examples in this file, above. Done. https://codereview.chromium.org/1302133002/diff/1/src/messages.h File src/messages.h (right): https://codereview.chromium.org/1302133002/diff/1/src/messages.h#newcode268 src/messages.h:268: T(InvalidTypedArrayIndex, "The value of Index is invalid") \ On 2015/08/20 18:23:34, bbudge wrote: > You should add a test in test/mjsunit/messages.js (search for 'simd' for an > example.) I noticed there is no test for the message on line above though. Looks like I probably should not have added this to begin with - changed to asserts and removed this. https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime-simd.cc File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime-simd.cc... src/runtime/runtime-simd.cc:830: FUNCTION(Int8x16, int8_t, 16) On 2015/08/20 18:23:34, bbudge wrote: > You'll have to partition these, since only the 4-lane types have LoadN / StoreN. Done. https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime.h File src/runtime/runtime.h (right): https://codereview.chromium.org/1302133002/diff/1/src/runtime/runtime.h#newco... src/runtime/runtime.h:693: F(Int16x8Load3, 2, 1) \ On 2015/08/20 18:23:34, bbudge wrote: > The spec no longer defines the LoadN / StoreN functions on types with 8 or 16 > lanes, so you can remove them here and below. Done.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302133002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302133002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/5079)
https://codereview.chromium.org/1302133002/diff/20001/src/harmony-simd.js File src/harmony-simd.js (right): https://codereview.chromium.org/1302133002/diff/20001/src/harmony-simd.js#new... src/harmony-simd.js:328: } You should be able to "macroize" these too. Parameterize on NAME and COUNT. https://codereview.chromium.org/1302133002/diff/20001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:839: RUNTIME_ASSERT(index >= 0); \ Combine this with the one below, i.e. index >= 0 && ... https://codereview.chromium.org/1302133002/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:844: RUNTIME_ASSERT(index* bpe + bytes <= byte_length); \ nit: space after 'index'
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/patch-status/1302133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302133002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/1302133002/diff/20001/src/harmony-simd.js File src/harmony-simd.js (right): https://codereview.chromium.org/1302133002/diff/20001/src/harmony-simd.js#new... src/harmony-simd.js:328: } On 2015/08/20 23:05:25, bbudge wrote: > You should be able to "macroize" these too. Parameterize on NAME and COUNT. Done. https://codereview.chromium.org/1302133002/diff/20001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:839: RUNTIME_ASSERT(index >= 0); \ On 2015/08/20 23:05:25, bbudge wrote: > Combine this with the one below, i.e. index >= 0 && ... Done. https://codereview.chromium.org/1302133002/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:844: RUNTIME_ASSERT(index* bpe + bytes <= byte_length); \ On 2015/08/20 23:05:26, bbudge wrote: > nit: space after 'index' Done.
https://codereview.chromium.org/1302133002/diff/40001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/40001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:839: Handle<JSTypedArray> tarray(JSTypedArray::cast(*tarray_obj)); \ I don't think this line is necessary (CONVERT_ARG_HANDLE_CHECKED gives you this handle.) Just change the name on that line to tarray.
https://codereview.chromium.org/1302133002/diff/40001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/40001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:839: Handle<JSTypedArray> tarray(JSTypedArray::cast(*tarray_obj)); \ On 2015/08/21 22:32:42, bbudge wrote: > I don't think this line is necessary (CONVERT_ARG_HANDLE_CHECKED gives you this > handle.) Just change the name on that line to tarray. Done.
The CQ bit was checked by gdeepti@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302133002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302133002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/nits Wait for review from Jarin and Dan https://codereview.chromium.org/1302133002/diff/60001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:835: Handle<type> result; \ nit: move Handle declaration and combine with last line. https://codereview.chromium.org/1302133002/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:847: memcpy(lanes, tarray_base + (index)*bpe, bytes); \ nit: spaces around '*' and remove parentheses.
The code looks good, but could you add some tests that verify the behavior? This could be in the form of somehow verifying that the polyfill's tests are executing against your load functions, rather than the polyfill-provided ones.
- Unsigned int types should also have load and store functions - To test, try removing the code in the simdjs tests which loads the polyfill, and run the tests anyway. At this point, we should have all functions implemented, right?
On 2015/08/22 01:17:08, Dan Ehrenberg wrote: > - Unsigned int types should also have load and store functions > - To test, try removing the code in the simdjs tests which loads the polyfill, > and run the tests anyway. At this point, we should have all functions > implemented, right? I haven't landed my upspec change, which adds the unsigned int types. I'll do that Monday morning when V8 starts rolling again. If Deepti expands this to include the store functions, then we should also be able to turn off the polyfill in the tests, so we'd be sure we were testing these.
On 2015/08/22 01:23:36, bbudge wrote: > On 2015/08/22 01:17:08, Dan Ehrenberg wrote: > > - Unsigned int types should also have load and store functions > > - To test, try removing the code in the simdjs tests which loads the polyfill, > > and run the tests anyway. At this point, we should have all functions > > implemented, right? > > I haven't landed my upspec change, which adds the unsigned int types. I'll do > that Monday morning when V8 starts rolling again. > > If Deepti expands this to include the store functions, then we should also be > able to turn off the polyfill in the tests, so we'd be sure we were testing > these. Forgot to add that a rebase to get the new types, and then adding the unsigned load/store functions would be necessary.
PTAL, added load functions for unsigned types. Verified the behavior on my local machine by turning off the polyfill. CL in progress to add store functions as well - the polyfill tests can then be turned off. https://codereview.chromium.org/1302133002/diff/60001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1302133002/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:835: Handle<type> result; \ On 2015/08/21 23:33:24, bbudge wrote: > nit: move Handle declaration and combine with last line. Done. https://codereview.chromium.org/1302133002/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:847: memcpy(lanes, tarray_base + (index)*bpe, bytes); \ On 2015/08/21 23:33:24, bbudge wrote: > nit: spaces around '*' and remove parentheses. Done.
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 Link to the patchset: https://codereview.chromium.org/1302133002/#ps80001 (title: "Rebase, adding unsigned load functions, review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302133002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302133002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
Still LGTM Please move declarations to before where they are first used in runtime-simd.cc. Also, could you make the title something like "Add SIMD load functions for Phase 1."?
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/patch-status/1302133002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302133002/100001
bbudge@chromium.org changed reviewers: + bmeurer@chromium.org - dehrenberg@chromium.org, gdeepti@google.com, jarin@chromium.org
lgtm
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org, bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/1302133002/#ps120001 (title: "Fixed format")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1302133002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1302133002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a6754d8c3c6541ac2f35cf151f963bdf1fe595ad Cr-Commit-Position: refs/heads/master@{#30422} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
