Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2909913002:
MIPS64: Add optimizations to li and Dsubu macro. (Closed)
|
Created:
3 years, 6 months ago by miran.karic Modified:
3 years, 6 months ago Reviewers:
ivica.bogosavljevic, predrag.rudic, ilija.pavlovic, sreten.kovacevic, miran.karic, Ilija.Pavlovic1, dusan.simicic CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS64: Add optimizations to li and Dsubu macro.
Here we optimize Dsubu by instead of loading imm and subtracting, we
load -imm and perform addition when loading -imm takes less instructions
than loading imm. Similarily li is optimized by loading -imm and
performing addition or loading ~imm and inverting bits using nor when
one of these loads takes two instructions less than loading imm, saving
at least one instruction. Tests are adjusted to cover these
optimizations.
BUG=
TEST=cctest/test-assembler-mips/li_macro
cctest/test-assembler-mips/Dsubu
Review-Url: https://codereview.chromium.org/2909913002
Cr-Commit-Position: refs/heads/master@{#46001}
Committed: https://chromium.googlesource.com/v8/v8/+/37b461a932362779dbe3dd8b9b0bc8adddfc0b29
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove DCHECK, adjust test. #Patch Set 3 : Rebase to master #
Messages
Total messages: 27 (14 generated)
Description was changed from
==========
MIPS64: Add optimizations to li and Dsubu macro.
Here we optimize Dsubu by instead of loading imm and subtracting, we
load -imm and perform addition when loading -imm takes less instructions
than loading imm. Similarily li is optimized by loading -imm and
performing addition or loading ~imm and inverting bits using nor when
one of these loads takes two instructions less than loading imm, saving
at least one instruction. Tests are adjusted to cover these
optimizations.
BUG=
TEST=cctest/test-assembler-mips/li_macro
cctest/test-assembler-mips/Dsubu
MIPS64: Fix missing parentheses failure.
Missing parentheses are causing compilation failures, added them.
BUG=
==========
to
==========
MIPS64: Add optimizations to li and Dsubu macro.
Here we optimize Dsubu by instead of loading imm and subtracting, we
load -imm and perform addition when loading -imm takes less instructions
than loading imm. Similarily li is optimized by loading -imm and
performing addition or loading ~imm and inverting bits using nor when
one of these loads takes two instructions less than loading imm, saving
at least one instruction. Tests are adjusted to cover these
optimizations.
BUG=
TEST=cctest/test-assembler-mips/li_macro
cctest/test-assembler-mips/Dsubu
MIPS64: Fix missing parentheses failure.
Missing parentheses are causing compilation failures, added them.
BUG=
==========
Remove following lines from description: MIPS64: Fix missing parentheses failure. Missing parentheses are causing compilation failures, added them. BUG=
Description was changed from
==========
MIPS64: Add optimizations to li and Dsubu macro.
Here we optimize Dsubu by instead of loading imm and subtracting, we
load -imm and perform addition when loading -imm takes less instructions
than loading imm. Similarily li is optimized by loading -imm and
performing addition or loading ~imm and inverting bits using nor when
one of these loads takes two instructions less than loading imm, saving
at least one instruction. Tests are adjusted to cover these
optimizations.
BUG=
TEST=cctest/test-assembler-mips/li_macro
cctest/test-assembler-mips/Dsubu
MIPS64: Fix missing parentheses failure.
Missing parentheses are causing compilation failures, added them.
BUG=
==========
to
==========
MIPS64: Add optimizations to li and Dsubu macro.
Here we optimize Dsubu by instead of loading imm and subtracting, we
load -imm and perform addition when loading -imm takes less instructions
than loading imm. Similarily li is optimized by loading -imm and
performing addition or loading ~imm and inverting bits using nor when
one of these loads takes two instructions less than loading imm, saving
at least one instruction. Tests are adjusted to cover these
optimizations.
BUG=
TEST=cctest/test-assembler-mips/li_macro
cctest/test-assembler-mips/Dsubu
==========
On 2017/05/29 16:24:36, dusan.simicic wrote: > Remove following lines from description: > > MIPS64: Fix missing parentheses failure. > > Missing parentheses are causing compilation failures, added them. > > BUG= Whoops, done.
lgtm
ilija.pavlovic@imgtec.com changed reviewers: + Ilija.Pavlovic@imgtec.com
lgtm
ivica.bogosavljevic@imgtec.com changed reviewers: - Ilija.Pavlovic@imgtec.com
https://codereview.chromium.org/2909913002/diff/1/src/mips64/macro-assembler-... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/2909913002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:665: DCHECK(rt.imm64_ != std::numeric_limits<int32_t>::min()); I don't understand why is this DCHECK here? If this situation cannot happen because of the condition in the if branch, then we don't need it. If the situation can happen, then it needs to be properly handled. https://codereview.chromium.org/2909913002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1852: if ((j.imm64_ & kUpper16MaskOf64) == 0 && is_int16(j.imm64_ >> 32) && this code is very complicated, but since we are optimizing for size then it is ok. But please: 1) make sure that the most common cases appear first 2) maka sure that for every case there is a test that covers it, both for the smallest possible value as well as for the largest possible value
https://codereview.chromium.org/2909913002/diff/1/src/mips64/macro-assembler-... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/2909913002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:665: DCHECK(rt.imm64_ != std::numeric_limits<int32_t>::min()); On 2017/06/05 12:32:12, ivica.bogosavljevic wrote: > I don't understand why is this DCHECK here? If this situation cannot happen > because of the condition in the if branch, then we don't need it. > > If the situation can happen, then it needs to be properly handled. I left it here so that when possible future changes are made to this code, the person doing the change would be aware of this case. But I guess I can remove it. It cannot happen because upper 16 bits are not 0 in this case which is checked in if condition. https://codereview.chromium.org/2909913002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1750: int MacroAssembler::InstrCountForLi64Bit(int64_t value) { Something I wanted to mention, this function has all the same conditions as the li_optimized function below. I tried to adjust the li_optimized function to add another parameter that would allow the function to either just return a number of instructions that if would emit or emit instructions. This way we wouldn't need to repeat conditions twice, but then we would need to check this parameter for every instruction that can be emitted. The code I got didn't look good even after using macro for the check and count or emit. But perhaps this is still something to consider. https://codereview.chromium.org/2909913002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1852: if ((j.imm64_ & kUpper16MaskOf64) == 0 && is_int16(j.imm64_ >> 32) && On 2017/06/05 12:32:11, ivica.bogosavljevic wrote: > this code is very complicated, but since we are optimizing for size then it is > ok. But please: > > 1) make sure that the most common cases appear first > 2) maka sure that for every case there is a test that covers it, both for the > smallest possible value as well as for the largest possible value This code remains the same as before, I only moved the li optimizations to a separate function so I can load -imm or ~imm as needed. Optimization cases are sorted so that cases that emit less instructions are checked sooner. Among those that emit the same number of instructions some adjustment could be made, do you think I should try running tests and check which optimizations are more commonly used? I will go through the test and see if there are cases that should be added. This is however tricky as border cases are often handled by another optimization before reaching the case observed so finding actual highest and lowest value used for a case can be hard.
Uploaded a new patch set, PTAL.
ilija.pavlovic@imgtec.com changed reviewers: + Ilija.Pavlovic@imgtec.com
lgtm
ivica.bogosavljevic@imgtec.com changed reviewers: - Ilija.Pavlovic@imgtec.com
lgtm
The CQ bit was checked by miran.karic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from dusan.simicic@imgtec.com Link to the patchset: https://codereview.chromium.org/2909913002/#ps20001 (title: "Remove DCHECK, adjust test.")
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
Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_gcc_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gcc_compile_dbg/...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/27480) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) 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/...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/24045) v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/43085)
The CQ bit was checked by miran.karic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from ivica.bogosavljevic@imgtec.com, Ilija.Pavlovic@imgtec.com, dusan.simicic@imgtec.com Link to the patchset: https://codereview.chromium.org/2909913002/#ps40001 (title: "Rebase to master")
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": 1497873311407280,
"parent_rev": "79fe6e3ec7deb1ac51cda942c8feb87c4274c9a3", "commit_rev":
"37b461a932362779dbe3dd8b9b0bc8adddfc0b29"}
Message was sent while issue was closed.
Description was changed from
==========
MIPS64: Add optimizations to li and Dsubu macro.
Here we optimize Dsubu by instead of loading imm and subtracting, we
load -imm and perform addition when loading -imm takes less instructions
than loading imm. Similarily li is optimized by loading -imm and
performing addition or loading ~imm and inverting bits using nor when
one of these loads takes two instructions less than loading imm, saving
at least one instruction. Tests are adjusted to cover these
optimizations.
BUG=
TEST=cctest/test-assembler-mips/li_macro
cctest/test-assembler-mips/Dsubu
==========
to
==========
MIPS64: Add optimizations to li and Dsubu macro.
Here we optimize Dsubu by instead of loading imm and subtracting, we
load -imm and perform addition when loading -imm takes less instructions
than loading imm. Similarily li is optimized by loading -imm and
performing addition or loading ~imm and inverting bits using nor when
one of these loads takes two instructions less than loading imm, saving
at least one instruction. Tests are adjusted to cover these
optimizations.
BUG=
TEST=cctest/test-assembler-mips/li_macro
cctest/test-assembler-mips/Dsubu
Review-Url: https://codereview.chromium.org/2909913002
Cr-Commit-Position: refs/heads/master@{#46001}
Committed:
https://chromium.googlesource.com/v8/v8/+/37b461a932362779dbe3dd8b9b0bc8adddf...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/37b461a932362779dbe3dd8b9b0bc8adddf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
