Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1965443003:
[simdjs] Implement error raising semantics as per spec for integers. (Closed)
Description[simdjs] Update SIMD runtime functions as per spec
- Lane indices are no longer required to be integers. Add index coersion for loads/stores
- Give shift operators masking shift count semantics
- Throw type/range errors instead of runtime asserts.
BUG=v8:4963
LOG=N
R=bbudge@chromium.org, bmeurer@chromium.org
Committed: https://crrev.com/4001d55e69162e40d1706ff479ac4db62fec6d90
Cr-Commit-Position: refs/heads/master@{#36402}
Patch Set 1 #Patch Set 2 : update to newer ecmascript_simd repository #
Total comments: 6
Patch Set 3 : Updating to new ecmascript_simd, fix tests #Patch Set 4 : Fix sign #
Total comments: 10
Patch Set 5 : Bill's review #
Total comments: 4
Patch Set 6 : Review changes #
Total comments: 2
Patch Set 7 : Review #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== [simdjs] Implement error raising semantics as per spec for integers. As per the current spec, lane indices are no longer required to be integers. Add index coersion and require the result to be an integer, throw type/range errors instead of runtime asserts. BUG=4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ========== to ========== [simdjs] Implement error raising semantics as per spec for integers. As per the current spec, lane indices are no longer required to be integers. Add index coersion and require the result to be an integer, throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ==========
Looks like testsuite needs adjustment?
https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:175: if (!name_object->IsNumber()) { \ Can't you assume name_object is a number at this point? https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:884: if (!name_object->IsNumber()) { \ Same comment as above. https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:897: SIMD_COERCE_INDEX(lane, 1); \ s/lane/index
Description was changed from ========== [simdjs] Implement error raising semantics as per spec for integers. As per the current spec, lane indices are no longer required to be integers. Add index coersion and require the result to be an integer, throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ========== to ========== [simdjs] Update SIMD runtime functions as per spec - Lane indices are no longer required to be integers. Add index coersion and for loads/stores - Give shift operators masking shift count semantics - Throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ==========
Description was changed from ========== [simdjs] Update SIMD runtime functions as per spec - Lane indices are no longer required to be integers. Add index coersion and for loads/stores - Give shift operators masking shift count semantics - Throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ========== to ========== [simdjs] Update SIMD runtime functions as per spec - Lane indices are no longer required to be integers. Add index coersion for loads/stores - Give shift operators masking shift count semantics - Throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ==========
https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:175: if (!name_object->IsNumber()) { \ On 2016/05/11 18:33:16, bbudge wrote: > Can't you assume name_object is a number at this point? Removed index coercion here as the polyfill does not support it yet. Gated on https://github.com/tc39/ecmascript_simd/pull/335 getting merged into the polyfill + polyfill being updated or using Test262 to run simd tests. Fixed for loads/stores. https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:884: if (!name_object->IsNumber()) { \ On 2016/05/11 18:33:16, bbudge wrote: > Same comment as above. Done. https://codereview.chromium.org/1965443003/diff/20001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:897: SIMD_COERCE_INDEX(lane, 1); \ On 2016/05/11 18:33:16, bbudge wrote: > s/lane/index Done.
https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:173: if (!name_object->IsNumber() || !IsInt32Double(name_object->Number())) { \ IsInt32Double rejects -0. Reading the spec, it looks like the conversion allows -0 (SameValueZero considers +0 and -0 equivalent) http://tc39.github.io/ecmascript_simd/#simd-to-lane Is the spec up to date? https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:175: isolate, NewTypeError(MessageTemplate::kInvalidSimdOperation)); \ It seems like kInvalidSimdIndex would be a more informative error. https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:410: if (!name_object->IsNumber() || !IsInt32Double(name_object->Number())) { \ From the spec, the conversion for shift counts is ToUint32: https://tc39.github.io/ecma262/#sec-touint32 https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:802: isolate, NewRangeError(MessageTemplate::kInvalidSimdIndex)); \ Invalid SIMD index doesn't seem right here since these are lane values. https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:884: if (name_object->Number() != std::floor(name_object->Number())) { \ The spec says the conversion is ToLength. http://tc39.github.io/ecmascript_simd/#simd-load-from-tarray
https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:173: if (!name_object->IsNumber() || !IsInt32Double(name_object->Number())) { \ On 2016/05/18 09:31:13, bbudge wrote: > IsInt32Double rejects -0. Reading the spec, it looks like the conversion allows > -0 (SameValueZero considers +0 and -0 equivalent) > > http://tc39.github.io/ecmascript_simd/#simd-to-lane > > Is the spec up to date? The Spec specifies ToNumber conversion, but that has not been implemented in the polyfill yet. Added that in my first patch but had to revert as tryjobs fail because we run tests against the polyfill. Just refactoring here to get rid of runtime asserts. Will update once the polyfill is in sync with spec or Test262 version is rolled forward and we can use that instead. Added a TODO. https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:175: isolate, NewTypeError(MessageTemplate::kInvalidSimdOperation)); \ On 2016/05/18 09:31:13, bbudge wrote: > It seems like kInvalidSimdIndex would be a more informative error. Done. https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:410: if (!name_object->IsNumber() || !IsInt32Double(name_object->Number())) { \ On 2016/05/18 09:31:13, bbudge wrote: > From the spec, the conversion for shift counts is ToUint32: > > https://tc39.github.io/ecma262/#sec-touint32 Not sure if I'm understanding this correctly, the conversion for shift counts is ToUint32 below, so the only thing required is to remove the IsInt32Double check as only IsNumber is required? https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:802: isolate, NewRangeError(MessageTemplate::kInvalidSimdIndex)); \ On 2016/05/18 09:31:13, bbudge wrote: > Invalid SIMD index doesn't seem right here since these are lane values. Done. https://codereview.chromium.org/1965443003/diff/60001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:884: if (name_object->Number() != std::floor(name_object->Number())) { \ On 2016/05/18 09:31:13, bbudge wrote: > The spec says the conversion is ToLength. > > http://tc39.github.io/ecmascript_simd/#simd-load-from-tarray Added index ≠ ToLength(index) check as per spec.
https://codereview.chromium.org/1965443003/diff/80001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1965443003/diff/80001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:178: uint32_t name = name_object->Number(); \ It would be better if you didn't have to call name_object->Number() twice here. Can you restructure it so IsIntDouble is checked together with name < 0 and Name >= lanes? https://codereview.chromium.org/1965443003/diff/80001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:800: from_ctype a_value = std::trunc(a->get_lane(i)); \ Can you move std::trunc into the appropriate CanCast specializations?
https://codereview.chromium.org/1965443003/diff/80001/src/runtime/runtime-sim... File src/runtime/runtime-simd.cc (right): https://codereview.chromium.org/1965443003/diff/80001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:178: uint32_t name = name_object->Number(); \ On 2016/05/19 14:14:44, bbudge wrote: > It would be better if you didn't have to call name_object->Number() twice here. > Can you restructure it so IsIntDouble is checked together with name < 0 and Name > >= lanes? Done. https://codereview.chromium.org/1965443003/diff/80001/src/runtime/runtime-sim... src/runtime/runtime-simd.cc:800: from_ctype a_value = std::trunc(a->get_lane(i)); \ On 2016/05/19 14:14:44, bbudge wrote: > Can you move std::trunc into the appropriate CanCast specializations? Done.
LGTM Thanks for fixing this! https://codereview.chromium.org/1965443003/diff/100001/src/messages.h File src/messages.h (right): https://codereview.chromium.org/1965443003/diff/100001/src/messages.h#newcode343 src/messages.h:343: T(InvalidSimdLaneValue, "SIMD lane value out of bounds for SIMD operation") \ nit: SIMD used twice, how about: Lane value out of bounds for SIMD operation?
https://codereview.chromium.org/1965443003/diff/100001/src/messages.h File src/messages.h (right): https://codereview.chromium.org/1965443003/diff/100001/src/messages.h#newcode343 src/messages.h:343: T(InvalidSimdLaneValue, "SIMD lane value out of bounds for SIMD operation") \ On 2016/05/20 09:36:00, bbudge wrote: > nit: SIMD used twice, how about: > Lane value out of bounds for SIMD operation? 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/1965443003/#ps120001 (title: "Review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965443003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965443003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/15640)
gdeepti@chromium.org changed reviewers: + littledan@chromium.org
lgtm
The CQ bit was checked by gdeepti@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965443003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965443003/120001
Message was sent while issue was closed.
Description was changed from ========== [simdjs] Update SIMD runtime functions as per spec - Lane indices are no longer required to be integers. Add index coersion for loads/stores - Give shift operators masking shift count semantics - Throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ========== to ========== [simdjs] Update SIMD runtime functions as per spec - Lane indices are no longer required to be integers. Add index coersion for loads/stores - Give shift operators masking shift count semantics - Throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [simdjs] Update SIMD runtime functions as per spec - Lane indices are no longer required to be integers. Add index coersion for loads/stores - Give shift operators masking shift count semantics - Throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org ========== to ========== [simdjs] Update SIMD runtime functions as per spec - Lane indices are no longer required to be integers. Add index coersion for loads/stores - Give shift operators masking shift count semantics - Throw type/range errors instead of runtime asserts. BUG=v8:4963 LOG=N R=bbudge@chromium.org, bmeurer@chromium.org Committed: https://crrev.com/4001d55e69162e40d1706ff479ac4db62fec6d90 Cr-Commit-Position: refs/heads/master@{#36402} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4001d55e69162e40d1706ff479ac4db62fec6d90 Cr-Commit-Position: refs/heads/master@{#36402} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
