Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1714283002:
MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.' (Closed)
DescriptionMIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.'
Port 1f5b84e467763510b08aac64577292205dfc19c
TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm
BUG=
Committed: https://crrev.com/50a394df2b844fb538a4859ca24ffb32cf9c81da
Cr-Commit-Position: refs/heads/master@{#34300}
Patch Set 1 #
Total comments: 7
Patch Set 2 : code refactoring. #
Total comments: 4
Patch Set 3 : code cleanup. #
Messages
Total messages: 16 (8 generated)
Description was changed from ========== MIPS: fix flaky test failures caused by CL 1522573002 BUG= ========== to ========== MIPS: fix flaky test failures caused by CL 1522573002 BUG= ==========
Description was changed from ========== MIPS: fix flaky test failures caused by CL 1522573002 BUG= ========== to ========== MIPS: fix LI macro failures in corner cases caused by CL 1522573002 TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ==========
Please start the description with MIPS: Fix ... and put a dot at the end. Tribute to Paul :) https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1384: } If I got it correctly at late evening then you use lui; ori for loading the lower 32 bits of the imm. But if the if (((imm >> kLuiShift) & kImm16Mask) == 0) then a daddiu(rd, zero_reg, (j.imm64_ & kImm16Mask)); is quicker/smaller then a lui(rd, 0); ori(rd, rd, j.imm64_ & kImm16Mask); Why don't you use the upper code block of if (is_int32(j.imm64_)) {} to load the lower 32 bits? In that case that code could be put in a helper function to keep the code DRY. https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1385: if ((imm >> 31) & 0x1) { Don't we have kImm32Mask? If not yet then I think that should be added. https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1398: } I guess the above if () {} else {} is for fixing sign extension (again). I would use a static inline function for that: // In comment: what is it good for. static inline int64_t fixSignExtension(int64_t imm, int bitnum) {...} ... imm = fixSignExtension(imm, 32); ... imm = fixSignExtension(imm, 16);
Description was changed from ========== MIPS: fix LI macro failures in corner cases caused by CL 1522573002 TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ========== to ========== MIPS: fix LI macro failures in corner cases caused by CL 1522573002. TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ==========
https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1384: } On 2016/02/23 19:56:23, balazs.kilvady wrote: > If I got it correctly at late evening then you use lui; ori for loading the > lower 32 bits of the imm. But if the > if (((imm >> kLuiShift) & kImm16Mask) == 0) then a > daddiu(rd, zero_reg, (j.imm64_ & kImm16Mask)); > is quicker/smaller then a lui(rd, 0); ori(rd, rd, j.imm64_ & kImm16Mask); > Why don't you use the upper code block of if (is_int32(j.imm64_)) {} to load the > lower 32 bits? In that case that code could be put in a helper function to keep > the code DRY. Done. https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1385: if ((imm >> 31) & 0x1) { Consolidated into shiftAndFixSignExtension. On 2016/02/23 19:56:23, balazs.kilvady wrote: > Don't we have kImm32Mask? If not yet then I think that should be added. https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1398: } On 2016/02/23 19:56:23, balazs.kilvady wrote: > I guess the above if () {} else {} is for fixing sign extension (again). I would > use a static inline function for that: > > // In comment: what is it good for. > static inline int64_t fixSignExtension(int64_t imm, int bitnum) {...} > ... > imm = fixSignExtension(imm, 32); > ... > imm = fixSignExtension(imm, 16); Done. https://codereview.chromium.org/1714283002/diff/1/src/mips64/macro-assembler-... src/mips64/macro-assembler-mips64.cc:1398: } On 2016/02/23 19:56:23, balazs.kilvady wrote: > I guess the above if () {} else {} is for fixing sign extension (again). I would > use a static inline function for that: > > // In comment: what is it good for. > static inline int64_t fixSignExtension(int64_t imm, int bitnum) {...} > ... > imm = fixSignExtension(imm, 32); > ... > imm = fixSignExtension(imm, 16); Done.
Please start the description with MIPS: Fix ... and put a dot at the end. Tribute to Paul :) The correct description would be: MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.' Port 1f5b84e467763510b08aac64577292205dfc19c TEST=<list of failing/flaky tests.> Also the description of the fixed CL was wrong but this time you should quote it as it is landed in the git repo. https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1361: static inline int64_t shiftAndFixSignExtension(int64_t imm, int bitnum) { Name should be ShiftAndFixSignExtension(). https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... File src/mips64/macro-assembler-mips64.h (right): https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.h:703: inline void liLower32BitHelper(Register rd, Operand j); Start the name with capital 'L'. Multi-word function names must be: WordWord(). Also I would not use inline. Compiler will optimize it if possible/useful.
Description was changed from ========== MIPS: fix LI macro failures in corner cases caused by CL 1522573002. TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ========== to ========== MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.' Port 1f5b84e467763510b08aac64577292205dfc19c TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ==========
https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... File src/mips64/macro-assembler-mips64.cc (right): https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.cc:1361: static inline int64_t shiftAndFixSignExtension(int64_t imm, int bitnum) { On 2016/02/25 14:14:03, balazs.kilvady wrote: > Name should be ShiftAndFixSignExtension(). Done. https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... File src/mips64/macro-assembler-mips64.h (right): https://codereview.chromium.org/1714283002/diff/20001/src/mips64/macro-assemb... src/mips64/macro-assembler-mips64.h:703: inline void liLower32BitHelper(Register rd, Operand j); On 2016/02/25 14:14:03, balazs.kilvady wrote: > Start the name with capital 'L'. Multi-word function names must be: WordWord(). > Also I would not use inline. Compiler will optimize it if possible/useful. Done.
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/1714283002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714283002/40001
Message was sent while issue was closed.
Description was changed from ========== MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.' Port 1f5b84e467763510b08aac64577292205dfc19c TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ========== to ========== MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.' Port 1f5b84e467763510b08aac64577292205dfc19c TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.' Port 1f5b84e467763510b08aac64577292205dfc19c TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= ========== to ========== MIPS64: Fix 'MIPS: use DAHI/DATH for li macro on mips64r6.' Port 1f5b84e467763510b08aac64577292205dfc19c TEST=test-run-machops/RunInt64SubWithOverflowImm, test-run-machops/RunInt64AddWithOverflowImm BUG= Committed: https://crrev.com/50a394df2b844fb538a4859ca24ffb32cf9c81da Cr-Commit-Position: refs/heads/master@{#34300} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/50a394df2b844fb538a4859ca24ffb32cf9c81da Cr-Commit-Position: refs/heads/master@{#34300} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
