Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1522573002:
MIPS: use DAHI/DATH for li macro on mips64r6. (Closed)
DescriptionMIPS: use DAHI/DATH for li macro on mips64r6.
BUG=
Committed: https://crrev.com/1f5b84e467763510b08aac64577292205dfc19c3
Cr-Commit-Position: refs/heads/master@{#34176}
Patch Set 1 #Patch Set 2 : Optimize for size. #Patch Set 3 : small fix. #Patch Set 4 : Optimize for size. #Patch Set 5 : positive int48 does not need dati. #
Total comments: 12
Patch Set 6 : copy shamelessly from Android kernel a better way to do LI. #
Total comments: 6
Patch Set 7 : Using a much simpler way to do LI. #Patch Set 8 : Adding tests for all LI flags. #
Total comments: 6
Patch Set 9 : more fixes. #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== MIPS: use DAHI/DATH for li macro on mips64r6. BUG= ========== to ========== MIPS: use DAHI/DATH for li macro on mips64r6. BUG= ==========
https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1298: int64_t imm = j.imm64_; This is the optimized case, so we need to get here with the least possible number of instruction. This code will emit two instructions (lui and dahi) regardless of the constant, but we can do better than that. I suggest you take a look at this implementation: https://android.googlesource.com/platform/art/+/master/compiler/utils/mips64/... https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1303: if ((imm >> 31) & 0x1) { Personally, I don't like this shifting bit magic since it is very complicated for me to understand. Look at the link from previous comment, you can see that instead of shifting the immediate values, they are adding constants to them. It is simpler to understand, and there are no complicated expressions like the one below: if (!((j.imm64_ >> 48 == 0xffff && (j.imm64_ > 47) & 0x1) || (j.imm64_ >> 48 == 0x0000 && ((j.imm64_ > 47) & 0x1) == 0x0))) { Consider using this approach https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1374: if (imm & kImm16Mask) { Looks good https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1390: lui(rd, (j.imm64_ >> 48) & kImm16Mask); This is the old code, nothing is changed here except for indentation. https://codereview.chromium.org/1522573002/diff/80001/test/cctest/test-assemb... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/80001/test/cctest/test-assemb... test/cctest/test-assembler-mips64.cc:5016: __ li(a0, rs); There is a third parametar to li macro called LiFlags which has three possible values: OPTIMIZE_SIZE, CONSTANT_SIZE and ADDRESS_LOAD. You need to test all three of them to get the full coverage of the code you wrote https://codereview.chromium.org/1522573002/diff/80001/test/cctest/test-assemb... test/cctest/test-assembler-mips64.cc:5040: 0x0000000000000000, Looks good
https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1298: int64_t imm = j.imm64_; On 2015/12/21 16:15:55, ivica.bogosavljevic wrote: > This is the optimized case, so we need to get here with the least possible > number of instruction. This code will emit two instructions (lui and dahi) > regardless of the constant, but we can do better than that. > I suggest you take a look at this implementation: > https://android.googlesource.com/platform/art/+/master/compiler/utils/mips64/... Done. https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1303: if ((imm >> 31) & 0x1) { On 2015/12/21 16:15:56, ivica.bogosavljevic wrote: > Personally, I don't like this shifting bit magic since it is very complicated > for me to understand. Look at the link from previous comment, you can see that > instead of shifting the immediate values, they are adding constants to them. It > is simpler to understand, and there are no complicated expressions like the one > below: > if (!((j.imm64_ >> 48 == 0xffff && (j.imm64_ > 47) & 0x1) || > (j.imm64_ >> 48 == 0x0000 && ((j.imm64_ > 47) & 0x1) == 0x0))) { > > Consider using this approach Done. https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1374: if (imm & kImm16Mask) { On 2015/12/21 16:15:56, ivica.bogosavljevic wrote: > Looks good Acknowledged. https://codereview.chromium.org/1522573002/diff/80001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1390: lui(rd, (j.imm64_ >> 48) & kImm16Mask); The old code is used in non-r6 architecture. On 2015/12/21 16:15:55, ivica.bogosavljevic wrote: > This is the old code, nothing is changed here except for indentation. https://codereview.chromium.org/1522573002/diff/80001/test/cctest/test-assemb... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/80001/test/cctest/test-assemb... test/cctest/test-assembler-mips64.cc:5016: __ li(a0, rs); On 2015/12/21 16:15:56, ivica.bogosavljevic wrote: > There is a third parametar to li macro called LiFlags which has three possible > values: OPTIMIZE_SIZE, CONSTANT_SIZE and ADDRESS_LOAD. You need to test all > three of them to get the full coverage of the code you wrote Done. https://codereview.chromium.org/1522573002/diff/80001/test/cctest/test-assemb... test/cctest/test-assembler-mips64.cc:5040: 0x0000000000000000, On 2015/12/21 16:15:56, ivica.bogosavljevic wrote: > Looks good Acknowledged.
https://codereview.chromium.org/1522573002/diff/100001/test/cctest/test-assem... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/100001/test/cctest/test-assem... test/cctest/test-assembler-mips64.cc:5005: __ li(a0, rs); You are still missing here the third argument https://codereview.chromium.org/1522573002/diff/100001/test/cctest/test-assem... test/cctest/test-assembler-mips64.cc:5007: __ jr(ra); This test fails. I tried compiling for v8 simulator and executing it. It fails with following problem: # # Fatal error in ../test/cctest/test-assembler-mips64.cc, line 5037 # Check failed: inputs[i] == res (18446462603027742720 vs. 18446462603027873792). # ==== C stack trace =============================== You will need to run quick check test for both r6 simulator and r6 qemu to make sure there are no regressions, since this change influences a lot of code
https://codereview.chromium.org/1522573002/diff/100001/src/mips64/macro-assem... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/100001/src/mips64/macro-assem... src/mips64/macro-assembler-mips64.cc:1326: if ((j.imm64_ & 0xffff0000) == 0 && is_int16(j.imm64_ >> 32)) { I am looking at this code and it's simply too complicated to be understood without spending 2 hours on it. So I would recommend a different approach instead of this monster (I tried it on my machine, and it's possible). You start from lui/ori/dahi/dati sequence from unoptimized version (it's bellow). If argument for lui is 0, skip lui If argument for ori is 0, skip ori If argument for dahi is 0, skip dahi If argument for dati is 0, skip dati. This should work in all cases except in case where input value is zero. In that case, after trying to generate lui and ori, none of these two instructions are generated, then simply load zero using or_(rd, zero_reg, zero_reg)
https://codereview.chromium.org/1522573002/diff/100001/src/mips64/macro-assem... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/100001/src/mips64/macro-assem... src/mips64/macro-assembler-mips64.cc:1326: if ((j.imm64_ & 0xffff0000) == 0 && is_int16(j.imm64_ >> 32)) { in the last commit I use this easy way to do the loading. On 2016/01/21 14:47:24, ivica.bogosavljevic wrote: > I am looking at this code and it's simply too complicated to be understood > without spending 2 hours on it. > > So I would recommend a different approach instead of this monster (I tried it on > my machine, and it's possible). > > You start from lui/ori/dahi/dati sequence from unoptimized version (it's > bellow). > > If argument for lui is 0, skip lui > If argument for ori is 0, skip ori > If argument for dahi is 0, skip dahi > If argument for dati is 0, skip dati. > > This should work in all cases except in case where input value is zero. In that > case, after trying to generate lui and ori, none of these two instructions are > generated, then simply load zero using or_(rd, zero_reg, zero_reg) https://codereview.chromium.org/1522573002/diff/100001/test/cctest/test-assem... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/100001/test/cctest/test-assem... test/cctest/test-assembler-mips64.cc:5005: __ li(a0, rs); On 2016/01/21 14:02:54, ivica.bogosavljevic wrote: > You are still missing here the third argument Done. https://codereview.chromium.org/1522573002/diff/100001/test/cctest/test-assem... test/cctest/test-assembler-mips64.cc:5007: __ jr(ra); On 2016/01/21 14:02:54, ivica.bogosavljevic wrote: > This test fails. I tried compiling for v8 simulator and executing it. It fails > with following problem: > > > # > # Fatal error in ../test/cctest/test-assembler-mips64.cc, line 5037 > # Check failed: inputs[i] == res (18446462603027742720 vs. > 18446462603027873792). > # > > ==== C stack trace =============================== > > > You will need to run quick check test for both r6 simulator and r6 qemu to make > sure there are no regressions, since this change influences a lot of code Done.
https://codereview.chromium.org/1522573002/diff/140001/src/mips64/macro-assem... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/140001/src/mips64/macro-assem... src/mips64/macro-assembler-mips64.cc:1323: if (kArchVariant == kMips64r6) { I tried this way, and it worked for me, all the test passed on simulator. Basically it's the same as your code for loading address, but before generating instruction, we check if it's parameter is zero and skip it: int64_t imm = j.imm64_; bool lui_emited = false; if (((imm >> kLuiShift) & kImm16Mask) != 0) { lui(rd, (imm >> kLuiShift) & kImm16Mask); lui_emited = true; } if ((imm & kImm16Mask) != 0) { ori(rd, rd, (imm & kImm16Mask)); } else if (!lui_emited) { // We didn't emit either lui or ori. // Need to load zero to rd or_(rd, zero_reg, zero_reg); } if ((imm >> 31) & 0x1) { imm = (imm >> 32) + 1; } else { imm = imm >> 32; } if ((imm & kImm16Mask) != 0) { dahi(rd, imm & kImm16Mask); } if ((imm >> 15) & 0x1) { imm = (imm >> 16) + 1; } else { imm = imm >> 16; } if ((imm & kImm16Mask) != 0) { dati(rd, imm & kImm16Mask); } https://codereview.chromium.org/1522573002/diff/140001/src/mips64/macro-assem... src/mips64/macro-assembler-mips64.cc:1400: lui(rd, (j.imm64_ >> 48) & kImm16Mask); What happened to Mips64R6 case in this patch? It got removed. https://codereview.chromium.org/1522573002/diff/140001/test/cctest/test-assem... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/140001/test/cctest/test-assem... test/cctest/test-assembler-mips64.cc:5036: uint64_t res = run_li_macro(inputs[i], OPTIMIZE_SIZE); Looks good
tests passed. Addressed Ivica's comments. https://codereview.chromium.org/1522573002/diff/140001/src/mips64/macro-assem... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/140001/src/mips64/macro-assem... src/mips64/macro-assembler-mips64.cc:1323: if (kArchVariant == kMips64r6) { I integrated the LUI_emit part in the latest code. On 2016/01/22 12:35:49, ivica.bogosavljevic wrote: > I tried this way, and it worked for me, all the test passed on simulator. > Basically it's the same as your code for loading address, but before generating > instruction, we check if it's parameter is zero and skip it: > > int64_t imm = j.imm64_; > bool lui_emited = false; > if (((imm >> kLuiShift) & kImm16Mask) != 0) { > lui(rd, (imm >> kLuiShift) & kImm16Mask); > lui_emited = true; > } > > if ((imm & kImm16Mask) != 0) { > ori(rd, rd, (imm & kImm16Mask)); > } else if (!lui_emited) { > // We didn't emit either lui or ori. > // Need to load zero to rd > or_(rd, zero_reg, zero_reg); > } > > if ((imm >> 31) & 0x1) { > imm = (imm >> 32) + 1; > } else { > imm = imm >> 32; > } > > if ((imm & kImm16Mask) != 0) { > dahi(rd, imm & kImm16Mask); > } > if ((imm >> 15) & 0x1) { > imm = (imm >> 16) + 1; > } else { > imm = imm >> 16; > } > > if ((imm & kImm16Mask) != 0) { > dati(rd, imm & kImm16Mask); > } https://codereview.chromium.org/1522573002/diff/140001/src/mips64/macro-assem... src/mips64/macro-assembler-mips64.cc:1400: lui(rd, (j.imm64_ >> 48) & kImm16Mask); I accidentally merged a previous version and got this removed. Now it is back. On 2016/01/22 12:35:49, ivica.bogosavljevic wrote: > What happened to Mips64R6 case in this patch? It got removed. https://codereview.chromium.org/1522573002/diff/140001/test/cctest/test-assem... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1522573002/diff/140001/test/cctest/test-assem... test/cctest/test-assembler-mips64.cc:5036: uint64_t res = run_li_macro(inputs[i], OPTIMIZE_SIZE); On 2016/01/22 12:35:49, ivica.bogosavljevic wrote: > Looks good Acknowledged.
lgtm
The CQ bit was checked by alan.li@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522573002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522573002/160001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
LGTM
The CQ bit was checked by alan.li@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1522573002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1522573002/160001
Message was sent while issue was closed.
Description was changed from ========== MIPS: use DAHI/DATH for li macro on mips64r6. BUG= ========== to ========== MIPS: use DAHI/DATH for li macro on mips64r6. BUG= ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== MIPS: use DAHI/DATH for li macro on mips64r6. BUG= ========== to ========== MIPS: use DAHI/DATH for li macro on mips64r6. BUG= Committed: https://crrev.com/1f5b84e467763510b08aac64577292205dfc19c3 Cr-Commit-Position: refs/heads/master@{#34176} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1f5b84e467763510b08aac64577292205dfc19c3 Cr-Commit-Position: refs/heads/master@{#34176} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
