Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907363002:
MIPS64: Fix [turbofan] Length and index2 are unsigned in CheckedLoad/CheckedStore. (Closed)
|
Created:
4 years, 8 months ago by balazs.kilvady Modified:
4 years, 7 months ago 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. |
DescriptionMIPS64: Fix [turbofan] Length and index2 are unsigned in CheckedLoad/CheckedStore.
Port b994ad45b089c64eb253f63526723ffc2a0c86c6
Original commit message:
Also factor out test cases from test-run-machops.cc into test-run-load-store.cc
TEST=cctest/test-run-load-store/RunLoadStoreZeroExtend64, cctest/test-run-load-store/RunOobCheckedLoadT_pseudo7, cctest/test-run-load-store/RunOobCheckedLoad_pseudo7
BUG=chromium:599717
LOG=Y
Committed: https://crrev.com/7551eca9816a7386282789e806bafedb9d4817bd
Cr-Commit-Position: refs/heads/master@{#36017}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased. #
Messages
Total messages: 15 (4 generated)
PTAL, I had to add
case MachineRepresentation::kWord32:
opcode = load_rep.IsUnsigned() ? kMips64Lwu : kMips64Lw;
in InstructionSelector::VisitLoad() to pass
cctest/test-run-load-store/RunLoadStoreZeroExtend64. There is no signed/unsigned
case on arm64 for kWord32 in VisitLoad() so I am not sure if this solution is
reliable and why arm64 and x64 can avoid unsigned case for Word32 load.
https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... File src/compiler/mips64/code-generator-mips64.cc (right): https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... src/compiler/mips64/code-generator-mips64.cc:369: __ And(kScratchReg, offset, Operand(0xffffffff)); \ We've been making the assumption on the other 64-bit platforms that the upper 32-bits of registers are cleared by 32-bit arithmetic operations. That's the case on x64 and arm64. Is that not the case on MIPS64?
https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... File src/compiler/mips64/code-generator-mips64.cc (right): https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... src/compiler/mips64/code-generator-mips64.cc:369: __ And(kScratchReg, offset, Operand(0xffffffff)); \ On 2016/04/25 08:44:10, titzer wrote: > We've been making the assumption on the other 64-bit platforms that the upper > 32-bits of registers are cleared by 32-bit arithmetic operations. That's the > case on x64 and arm64. Is that not the case on MIPS64? Here offset is a register. In that case if offset got its vale with a 'lwu offset, ....' (unsigned load) then the upper 32 bits zeroed (i.e. the value is zero extended -- unsigned). If offfet is loaded with a 'lw offset, ....' then the value is sign extended. As I saw from the logs/debugs of failures the later case holds so a masking helps at this point. But I feel that offset should be unsigned and loaded with lwu. But I have not fond yet where to fix/ensure it. I would be happy if you can give some hints :)
On 2016/04/25 10:49:19, balazs.kilvady wrote: > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > File src/compiler/mips64/code-generator-mips64.cc (right): > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > src/compiler/mips64/code-generator-mips64.cc:369: __ And(kScratchReg, offset, > Operand(0xffffffff)); \ > On 2016/04/25 08:44:10, titzer wrote: > > We've been making the assumption on the other 64-bit platforms that the upper > > 32-bits of registers are cleared by 32-bit arithmetic operations. That's the > > case on x64 and arm64. Is that not the case on MIPS64? > > Here offset is a register. In that case if offset got its vale with a 'lwu > offset, ....' (unsigned load) then the upper 32 bits zeroed (i.e. the value is > zero extended -- unsigned). If offfet is loaded with a 'lw offset, ....' then > the value is sign extended. As I saw from the logs/debugs of failures the later > case holds so a masking helps at this point. But I feel that offset should be > unsigned and loaded with lwu. But I have not fond yet where to fix/ensure it. I > would be happy if you can give some hints :) It's an invariant preserved by all arithmetic. The offset may be computed by some 32-bit arithmetic elsewhere. On x64 and arm64, all 32-bit arithmetic instructions clear the upper bits of the destination register. We even have assertions to catch cases where it does not hold. Is this also possible on MIPS64?
On 2016/04/25 11:03:41, titzer wrote: > On 2016/04/25 10:49:19, balazs.kilvady wrote: > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > File src/compiler/mips64/code-generator-mips64.cc (right): > > > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > src/compiler/mips64/code-generator-mips64.cc:369: __ And(kScratchReg, offset, > > Operand(0xffffffff)); \ > > On 2016/04/25 08:44:10, titzer wrote: > > > We've been making the assumption on the other 64-bit platforms that the > upper > > > 32-bits of registers are cleared by 32-bit arithmetic operations. That's the > > > case on x64 and arm64. Is that not the case on MIPS64? > > > > Here offset is a register. In that case if offset got its vale with a 'lwu > > offset, ....' (unsigned load) then the upper 32 bits zeroed (i.e. the value is > > zero extended -- unsigned). If offfet is loaded with a 'lw offset, ....' then > > the value is sign extended. As I saw from the logs/debugs of failures the > later > > case holds so a masking helps at this point. But I feel that offset should be > > unsigned and loaded with lwu. But I have not fond yet where to fix/ensure it. > I > > would be happy if you can give some hints :) > > It's an invariant preserved by all arithmetic. The offset may be computed by > some 32-bit arithmetic elsewhere. On x64 and arm64, all 32-bit arithmetic > instructions clear the upper bits of the destination register. We even have > assertions to catch cases where it does not hold. Is this also possible on > MIPS64? We have nice logical and arithmetic shifts. But the mulu/muhu and some others for unsigned ops, but the result is always sign-extended like at addu: Format: ADDU rd, rs, rt Purpose: Add Unsigned Word To add 32-bit integers. Description: GPR[rd] <- GPR[rs] + GPR[rt] The 32-bit word value in GPR rt is added to the 32-bit value in GPR rs and the 32-bit arithmetic result is sign- extended and placed into GPR rd.
On 2016/04/25 12:05:43, balazs.kilvady wrote: > On 2016/04/25 11:03:41, titzer wrote: > > On 2016/04/25 10:49:19, balazs.kilvady wrote: > > > > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > > File src/compiler/mips64/code-generator-mips64.cc (right): > > > > > > > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > > src/compiler/mips64/code-generator-mips64.cc:369: __ And(kScratchReg, > offset, > > > Operand(0xffffffff)); \ > > > On 2016/04/25 08:44:10, titzer wrote: > > > > We've been making the assumption on the other 64-bit platforms that the > > upper > > > > 32-bits of registers are cleared by 32-bit arithmetic operations. That's > the > > > > case on x64 and arm64. Is that not the case on MIPS64? > > > > > > Here offset is a register. In that case if offset got its vale with a 'lwu > > > offset, ....' (unsigned load) then the upper 32 bits zeroed (i.e. the value > is > > > zero extended -- unsigned). If offfet is loaded with a 'lw offset, ....' > then > > > the value is sign extended. As I saw from the logs/debugs of failures the > > later > > > case holds so a masking helps at this point. But I feel that offset should > be > > > unsigned and loaded with lwu. But I have not fond yet where to fix/ensure > it. > > I > > > would be happy if you can give some hints :) > > > > It's an invariant preserved by all arithmetic. The offset may be computed by > > some 32-bit arithmetic elsewhere. On x64 and arm64, all 32-bit arithmetic > > instructions clear the upper bits of the destination register. We even have > > assertions to catch cases where it does not hold. Is this also possible on > > MIPS64? > > We have nice logical and arithmetic shifts. But the mulu/muhu and some others > for unsigned ops, but the result is always sign-extended like at addu: > Format: ADDU rd, rs, rt > Purpose: Add Unsigned Word > To add 32-bit integers. > Description: GPR[rd] <- GPR[rs] + GPR[rt] > The 32-bit word value in GPR rt is added to the 32-bit value in GPR rs and the > 32-bit arithmetic result is sign- > extended and placed into GPR rd. If you think it would be useful and not cause too much overhead then we could add unsigned (masked) instructions to MacroAssembler (like AddUnsigned, SubUnsigned and so on) to hide and ensure this kind of masking which is used in this CL for 'offset'.
On 2016/04/26 10:05:52, balazs.kilvady wrote: > On 2016/04/25 12:05:43, balazs.kilvady wrote: > > On 2016/04/25 11:03:41, titzer wrote: > > > On 2016/04/25 10:49:19, balazs.kilvady wrote: > > > > > > > > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > > > File src/compiler/mips64/code-generator-mips64.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > > > src/compiler/mips64/code-generator-mips64.cc:369: __ And(kScratchReg, > > offset, > > > > Operand(0xffffffff)); \ > > > > On 2016/04/25 08:44:10, titzer wrote: > > > > > We've been making the assumption on the other 64-bit platforms that the > > > upper > > > > > 32-bits of registers are cleared by 32-bit arithmetic operations. That's > > the > > > > > case on x64 and arm64. Is that not the case on MIPS64? > > > > > > > > Here offset is a register. In that case if offset got its vale with a 'lwu > > > > offset, ....' (unsigned load) then the upper 32 bits zeroed (i.e. the > value > > is > > > > zero extended -- unsigned). If offfet is loaded with a 'lw offset, ....' > > then > > > > the value is sign extended. As I saw from the logs/debugs of failures the > > > later > > > > case holds so a masking helps at this point. But I feel that offset should > > be > > > > unsigned and loaded with lwu. But I have not fond yet where to fix/ensure > > it. > > > I > > > > would be happy if you can give some hints :) > > > > > > It's an invariant preserved by all arithmetic. The offset may be computed by > > > some 32-bit arithmetic elsewhere. On x64 and arm64, all 32-bit arithmetic > > > instructions clear the upper bits of the destination register. We even have > > > assertions to catch cases where it does not hold. Is this also possible on > > > MIPS64? > > > > We have nice logical and arithmetic shifts. But the mulu/muhu and some others > > for unsigned ops, but the result is always sign-extended like at addu: > > Format: ADDU rd, rs, rt > > Purpose: Add Unsigned Word > > To add 32-bit integers. > > Description: GPR[rd] <- GPR[rs] + GPR[rt] > > The 32-bit word value in GPR rt is added to the 32-bit value in GPR rs and the > > 32-bit arithmetic result is sign- > > extended and placed into GPR rd. > > If you think it would be useful and not cause too much overhead then we could > add unsigned (masked) instructions to MacroAssembler (like AddUnsigned, > SubUnsigned and so on) to hide and ensure this kind of masking which is used in > this CL for 'offset'. Since there are no hardware instructions that do 32-bit arithmetic and zero the upper bits, I recommend staying with this approach which masks the offsets in the checked loads/stores. They should be far less frequent than arithmetic.
On 2016/05/03 13:27:45, titzer wrote: > On 2016/04/26 10:05:52, balazs.kilvady wrote: > > On 2016/04/25 12:05:43, balazs.kilvady wrote: > > > On 2016/04/25 11:03:41, titzer wrote: > > > > On 2016/04/25 10:49:19, balazs.kilvady wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > > > > File src/compiler/mips64/code-generator-mips64.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1907363002/diff/1/src/compiler/mips64/code-ge... > > > > > src/compiler/mips64/code-generator-mips64.cc:369: __ And(kScratchReg, > > > offset, > > > > > Operand(0xffffffff)); \ > > > > > On 2016/04/25 08:44:10, titzer wrote: > > > > > > We've been making the assumption on the other 64-bit platforms that > the > > > > upper > > > > > > 32-bits of registers are cleared by 32-bit arithmetic operations. > That's > > > the > > > > > > case on x64 and arm64. Is that not the case on MIPS64? > > > > > > > > > > Here offset is a register. In that case if offset got its vale with a > 'lwu > > > > > offset, ....' (unsigned load) then the upper 32 bits zeroed (i.e. the > > value > > > is > > > > > zero extended -- unsigned). If offfet is loaded with a 'lw offset, ....' > > > then > > > > > the value is sign extended. As I saw from the logs/debugs of failures > the > > > > later > > > > > case holds so a masking helps at this point. But I feel that offset > should > > > be > > > > > unsigned and loaded with lwu. But I have not fond yet where to > fix/ensure > > > it. > > > > I > > > > > would be happy if you can give some hints :) > > > > > > > > It's an invariant preserved by all arithmetic. The offset may be computed > by > > > > some 32-bit arithmetic elsewhere. On x64 and arm64, all 32-bit arithmetic > > > > instructions clear the upper bits of the destination register. We even > have > > > > assertions to catch cases where it does not hold. Is this also possible on > > > > MIPS64? > > > > > > We have nice logical and arithmetic shifts. But the mulu/muhu and some > others > > > for unsigned ops, but the result is always sign-extended like at addu: > > > Format: ADDU rd, rs, rt > > > Purpose: Add Unsigned Word > > > To add 32-bit integers. > > > Description: GPR[rd] <- GPR[rs] + GPR[rt] > > > The 32-bit word value in GPR rt is added to the 32-bit value in GPR rs and > the > > > 32-bit arithmetic result is sign- > > > extended and placed into GPR rd. > > > > If you think it would be useful and not cause too much overhead then we could > > add unsigned (masked) instructions to MacroAssembler (like AddUnsigned, > > SubUnsigned and so on) to hide and ensure this kind of masking which is used > in > > this CL for 'offset'. > > Since there are no hardware instructions that do 32-bit arithmetic and zero the > upper bits, I recommend staying with this approach which masks the offsets in > the checked loads/stores. They should be far less frequent than arithmetic. lgtm
The CQ bit was checked by balazs.kilvady@imgtec.com
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/1907363002/#ps20001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907363002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MIPS64: Fix [turbofan] Length and index2 are unsigned in CheckedLoad/CheckedStore. Port b994ad45b089c64eb253f63526723ffc2a0c86c6 Original commit message: Also factor out test cases from test-run-machops.cc into test-run-load-store.cc TEST=cctest/test-run-load-store/RunLoadStoreZeroExtend64, cctest/test-run-load-store/RunOobCheckedLoadT_pseudo7, cctest/test-run-load-store/RunOobCheckedLoad_pseudo7 BUG=chromium:599717 LOG=Y ========== to ========== MIPS64: Fix [turbofan] Length and index2 are unsigned in CheckedLoad/CheckedStore. Port b994ad45b089c64eb253f63526723ffc2a0c86c6 Original commit message: Also factor out test cases from test-run-machops.cc into test-run-load-store.cc TEST=cctest/test-run-load-store/RunLoadStoreZeroExtend64, cctest/test-run-load-store/RunOobCheckedLoadT_pseudo7, cctest/test-run-load-store/RunOobCheckedLoad_pseudo7 BUG=chromium:599717 LOG=Y Committed: https://crrev.com/7551eca9816a7386282789e806bafedb9d4817bd Cr-Commit-Position: refs/heads/master@{#36017} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7551eca9816a7386282789e806bafedb9d4817bd Cr-Commit-Position: refs/heads/master@{#36017} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
