Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784253002:
MIPS: Fix `[builtins] Reland of Port TypedArrayInitialize to CodeStubAssembler.` (Closed)
|
Created:
3 years, 8 months ago by ivica.bogosavljevic Modified:
3 years, 8 months ago CC:
v8-mips-ports_googlegroups.com, v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS: Fix `[builtins] Reland of Port TypedArrayInitialize to CodeStubAssembler.`
Fix ff8b1abb1a8e609b7786d0f6cd6577cc9083d262
This fixes the problem with the alignment of typed arrays in turbofan. Namely,
Float64 typed arrays weren't properly aligned on 32bit architectures,
and this causes crashes on those architectures that do not support misaligned
memory access.
TEST=mjsunit/es6/typedarray-*
BUG=v8:6075
Review-Url: https://codereview.chromium.org/2784253002
Cr-Commit-Position: refs/heads/master@{#44366}
Committed: https://chromium.googlesource.com/v8/v8/+/74b8ef6cea108d55d479afe16ffa2b0e7bf90074
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address code review remarks. #Patch Set 3 : Fix failure on CSA bot #
Messages
Total messages: 29 (19 generated)
ivica.bogosavljevic@imgtec.com changed reviewers: + cbruni@chromium.org, epertoso@chromium.org, petermarshall@chromium.org
PTAL This enables double alignment for typed array allocation and also fixes some issues in the alignment code that caused failures when enabled.
LGTM, can you wait for cbruni to review as well? Thanks!
On 2017/03/30 11:51:36, petermarshall wrote: > LGTM, can you wait for cbruni to review as well? Thanks! of course
The CQ bit was checked by ivica.bogosavljevic@imgtec.com 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_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
some more comments https://codereview.chromium.org/2784253002/diff/1/src/builtins/builtins-typed... File src/builtins/builtins-typedarray-gen.cc (right): https://codereview.chromium.org/2784253002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray-gen.cc:230: Node* elements = Allocate(total_size.value(), kDoubleAlignment); not: also change this to AllocateInNewSpace(...) https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc#... src/code-stub-assembler.cc:698: Smi::FromInt(AllocateDoubleAlignFlag::encode(flags & kDoubleAlignment) | please add bool needs_double_alignment = flags & kDoubleAlignment; and replace users in this method. https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc#... src/code-stub-assembler.cc:785: Node* limit_address) { CHECK((flags & kDoubleAlignment) == 0); For consistency: I don't think we should have the kDoubleAlignment flag set if we want to allocate unaligned memory. https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc#... src/code-stub-assembler.cc:786: return AllocateRaw(size_in_bytes, flags & ~kDoubleAlignment, top_address, ... and then just use flags here directly. https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc#... src/code-stub-assembler.cc:790: Node* CodeStubAssembler::AllocateRawAligned(Node* size_in_bytes, Let's make this fully obvious: can you rename this method to AllocateRawDoubleAligned ?
PTAL https://codereview.chromium.org/2784253002/diff/1/src/builtins/builtins-typed... File src/builtins/builtins-typedarray-gen.cc (right): https://codereview.chromium.org/2784253002/diff/1/src/builtins/builtins-typed... src/builtins/builtins-typedarray-gen.cc:230: Node* elements = Allocate(total_size.value(), kDoubleAlignment); On 2017/03/30 12:08:23, Camillo Bruni wrote: > not: also change this to AllocateInNewSpace(...) Acknowledged. https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc File src/code-stub-assembler.cc (right): https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc#... src/code-stub-assembler.cc:698: Smi::FromInt(AllocateDoubleAlignFlag::encode(flags & kDoubleAlignment) | On 2017/03/30 12:08:23, Camillo Bruni wrote: > please add > bool needs_double_alignment = flags & kDoubleAlignment; > and replace users in this method. Acknowledged. https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc#... src/code-stub-assembler.cc:785: Node* limit_address) { On 2017/03/30 12:08:23, Camillo Bruni wrote: > CHECK((flags & kDoubleAlignment) == 0); > > For consistency: I don't think we should have the kDoubleAlignment flag set if > we want to allocate unaligned memory. This is true but for 32bit architectures only. On 64 bit architectures, you can have kDoubleAlignment set and call AllocateRawUnaligned, this is actually done in Node* CodeStubAssembler::Allocate(Node* size_in_bytes, AllocationFlags flags), line 821. https://codereview.chromium.org/2784253002/diff/1/src/code-stub-assembler.cc#... src/code-stub-assembler.cc:790: Node* CodeStubAssembler::AllocateRawAligned(Node* size_in_bytes, On 2017/03/30 12:08:23, Camillo Bruni wrote: > Let's make this fully obvious: can you rename this method to > AllocateRawDoubleAligned ? Acknowledged.
The CQ bit was checked by ivica.bogosavljevic@imgtec.com 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_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by ivica.bogosavljevic@imgtec.com 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...
Applied changes PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/31 10:00:18, ivica.bogosavljevic wrote: > Applied changes PTAL ping...
Description was changed from ========== MIPS: Fix `[builtins] Reland of Port TypedArrayInitialize to CodeStubAssembler.` Fix ff8b1abb1a8e609b7786d0f6cd6577cc9083d262 This fixes the problem with the alignment of typed arrays in turbofan. Namely, Float64 typed arrays weren't properly aligned on 32bit architectures, and this causes crashes on those architectures that do not support misaligned memory access. TEST=mjsunit/es6/typedarray-* BUG= ========== to ========== MIPS: Fix `[builtins] Reland of Port TypedArrayInitialize to CodeStubAssembler.` Fix ff8b1abb1a8e609b7786d0f6cd6577cc9083d262 This fixes the problem with the alignment of typed arrays in turbofan. Namely, Float64 typed arrays weren't properly aligned on 32bit architectures, and this causes crashes on those architectures that do not support misaligned memory access. TEST=mjsunit/es6/typedarray-* BUG=v8:6075 ==========
petermarshall@chromium.org changed reviewers: - epertoso@chromium.org
Sorry for the late response and thanks for addressing my nits! LGTM
The CQ bit was checked by ivica.bogosavljevic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from petermarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2784253002/#ps40001 (title: "Fix failure on CSA bot")
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": 40001, "attempt_start_ts": 1491295989561780,
"parent_rev": "45e3c56d409e27ce7e555fe729503e13517e36f6", "commit_rev":
"74b8ef6cea108d55d479afe16ffa2b0e7bf90074"}
Message was sent while issue was closed.
Description was changed from ========== MIPS: Fix `[builtins] Reland of Port TypedArrayInitialize to CodeStubAssembler.` Fix ff8b1abb1a8e609b7786d0f6cd6577cc9083d262 This fixes the problem with the alignment of typed arrays in turbofan. Namely, Float64 typed arrays weren't properly aligned on 32bit architectures, and this causes crashes on those architectures that do not support misaligned memory access. TEST=mjsunit/es6/typedarray-* BUG=v8:6075 ========== to ========== MIPS: Fix `[builtins] Reland of Port TypedArrayInitialize to CodeStubAssembler.` Fix ff8b1abb1a8e609b7786d0f6cd6577cc9083d262 This fixes the problem with the alignment of typed arrays in turbofan. Namely, Float64 typed arrays weren't properly aligned on 32bit architectures, and this causes crashes on those architectures that do not support misaligned memory access. TEST=mjsunit/es6/typedarray-* BUG=v8:6075 Review-Url: https://codereview.chromium.org/2784253002 Cr-Commit-Position: refs/heads/master@{#44366} Committed: https://chromium.googlesource.com/v8/v8/+/74b8ef6cea108d55d479afe16ffa2b0e7bf... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/74b8ef6cea108d55d479afe16ffa2b0e7bf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
