Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2535703002:
MIPS: Optimize load/store with large offset on MIPSr6 (Closed)
|
Created:
4 years ago by Marija Antic Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS: Optimize load/store with large offset on MIPSr6
Replace the sequence LUI+(D)ADD with (D)AUI
BUG=
Committed: https://crrev.com/51159360d48b7871dd1e049581eee92a4844842a
Cr-Commit-Position: refs/heads/master@{#41425}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : fix errors #
Messages
Total messages: 19 (8 generated)
PTAL
https://codereview.chromium.org/2535703002/diff/1/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): https://codereview.chromium.org/2535703002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:1783: ori(at, at, src.offset_ & kImm16Mask); // Load 32-bit offset. It is possible to optimize this sequence of code as well. instead of LUI at, upper(src.offset) ORI at, lower(src.offset) ADDU at, at, src.rm we could write AUI at, src.rm, upper(src.offset) ADDIU at, at, lower(src.offset) https://codereview.chromium.org/2535703002/diff/1/src/mips64/assembler-mips64.cc File src/mips64/assembler-mips64.cc (right): https://codereview.chromium.org/2535703002/diff/1/src/mips64/assembler-mips64... src/mips64/assembler-mips64.cc:1943: lui(at, (src.offset_ >> kLuiShift) & kImm16Mask); Same remark as for MIPS32
https://codereview.chromium.org/2535703002/diff/1/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): https://codereview.chromium.org/2535703002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:1783: ori(at, at, src.offset_ & kImm16Mask); // Load 32-bit offset. On 2016/11/28 13:13:46, ivica.bogosavljevic wrote: > It is possible to optimize this sequence of code as well. > > instead of > LUI at, upper(src.offset) > ORI at, lower(src.offset) > ADDU at, at, src.rm > > we could write > > AUI at, src.rm, upper(src.offset) > ADDIU at, at, lower(src.offset) Done. https://codereview.chromium.org/2535703002/diff/1/src/mips64/assembler-mips64.cc File src/mips64/assembler-mips64.cc (right): https://codereview.chromium.org/2535703002/diff/1/src/mips64/assembler-mips64... src/mips64/assembler-mips64.cc:1943: lui(at, (src.offset_ >> kLuiShift) & kImm16Mask); On 2016/11/28 13:13:46, ivica.bogosavljevic wrote: > Same remark as for MIPS32 Done.
https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... File src/mips64/assembler-mips64.cc (right): https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... src/mips64/assembler-mips64.cc:1945: daui(at, src.rm(), hi); Does this compile? (hi is not defined anywhere)
https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... File src/mips64/assembler-mips64.cc (right): https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... src/mips64/assembler-mips64.cc:1945: daui(at, src.rm(), hi); On 2016/11/28 13:44:13, ivica.bogosavljevic wrote: > Does this compile? (hi is not defined anywhere) Acknowledged.
On 2016/11/29 12:17:04, Marija Antic wrote: > https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... > File src/mips64/assembler-mips64.cc (right): > > https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... > src/mips64/assembler-mips64.cc:1945: daui(at, src.rm(), hi); > On 2016/11/28 13:44:13, ivica.bogosavljevic wrote: > > Does this compile? (hi is not defined anywhere) > > Acknowledged. What was bad in set patch 2 except missing definition of variable hi?
On 2016/11/30 13:43:25, predrag.rudic wrote: > On 2016/11/29 12:17:04, Marija Antic wrote: > > > https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... > > File src/mips64/assembler-mips64.cc (right): > > > > > https://codereview.chromium.org/2535703002/diff/20001/src/mips64/assembler-mi... > > src/mips64/assembler-mips64.cc:1945: daui(at, src.rm(), hi); > > On 2016/11/28 13:44:13, ivica.bogosavljevic wrote: > > > Does this compile? (hi is not defined anywhere) > > > > Acknowledged. > > What was bad in set patch 2 except missing definition of variable hi? Sign extension of the immediate operands cause problems, which require checking if the lower part of the offset is negative, and if the sign of higher part would be changed (in case of mips64) after it is incremented.
lgtm
The CQ bit was checked by marija.antic@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: This issue passed the CQ dry run.
The CQ bit was checked by marija.antic@imgtec.com
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": 1480597717030500,
"parent_rev": "db660a4f1b736032390bdf4da03f427f4613dcf0", "commit_rev":
"031cedfbcca7b37b106dd7fdc1cbe819a4150176"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MIPS: Optimize load/store with large offset on MIPSr6 Replace the sequence LUI+(D)ADD with (D)AUI BUG= ========== to ========== MIPS: Optimize load/store with large offset on MIPSr6 Replace the sequence LUI+(D)ADD with (D)AUI BUG= Committed: https://crrev.com/51159360d48b7871dd1e049581eee92a4844842a Cr-Commit-Position: refs/heads/master@{#41425} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/51159360d48b7871dd1e049581eee92a4844842a Cr-Commit-Position: refs/heads/master@{#41425} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
