Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1573983002:
MIPS: Replace JR/JALR with JIC/JIALC for r6 (Closed)
|
Created:
4 years, 11 months ago by miran.karic Modified:
4 years, 1 month 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. |
DescriptionMIPS: Replace JR/JALR with JIC/JIALC for r6
This is the first step in process of replacing JR and JALR instructions
with JIC and JIALC for r6. Trampoline in r6 now uses JIC. Also
BranchLong and BranchAndLinkLong MacroAssembler functions now use JIC
and JIALC in r6 if branch delay slot is not used.
BUG=
Committed: https://crrev.com/c766f739ddd01b3688cfa94ec295b768f3edadf4
Cr-Commit-Position: refs/heads/master@{#34236}
Patch Set 1 #
Total comments: 24
Patch Set 2 : rebasing to master #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Messages
Total messages: 15 (5 generated)
Description was changed from ========== MIPS: Replace JR/JALR with JIC/JIALC for r6 This is the first step in process of replacing JR and JALR instructions with JIC and JIALC for r6. Trampoline in r6 now uses JIC. Also BranchLong and BranchAndLinkLong MacroAssembler functions now use JIC and JIALC in r6 if branch delay slot is not used. BUG= ========== to ========== MIPS: Replace JR/JALR with JIC/JIALC for r6 This is the first step in process of replacing JR and JALR instructions with JIC and JIALC for r6. Trampoline in r6 now uses JIC. Also BranchLong and BranchAndLinkLong MacroAssembler functions now use JIC and JIALC in r6 if branch delay slot is not used. BUG= ==========
l-g-t-m, really nice trick :)
https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips-inl.h File src/mips/assembler-mips-inl.h (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips-inl... src/mips/assembler-mips-inl.h:375: (Assembler::IsJal(instr2) || Assembler::IsJalr(instr2))) { I suspect that 'IsJal(instr2)' is not needed anymore after https://codereview.chromium.org/1419793014 and should be removed. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:726: uint32_t Assembler::createTargetAddress(Instr instr_lui, Instr instr_jic) { nit: Start with capital C: CreateTargetAddress, or change to all lower_case_with_underscore. Here and two places below. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:735: uint32_t lui_offsetU = ((uint32_t)lui_offset) << kLuiShift; nit: probably should not end in 'U', suggest '_u' https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:742: // Idea is to use just lui and jic instructions. We will insert lower nit: remove 'Idea is to ', and start with 'Use just ...' Also, please correct the word-wrapping of this comment block. There should not be single words on a line, except possibly the last line. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:872: if (IsJicOrJialc(instr2)) { Just for readability, can this if (IsJicOrJialc(instr2)) { } be combined with the one just below the instr2 &= ~kImm16Mask; ? It doesn't look like there are dependencies there. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:873: uint32_t lui_offsetU, jic_offsetU; nit: as before, don't use capital U at end like this. _u https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.h File src/mips/assembler-mips.h (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.h#n... src/mips/assembler-mips.h:526: // follows LUI/ORI pair is substituted with J/JAL, this constant equals These comments about J/JAL should have been removed in https://codereview.chromium.org/1419793014. Please remove them now. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.h#n... src/mips/assembler-mips.h:532: static const int kInstructionsFor32BitConstant = 3; I _think_ this case of 3 was only useful when we had J/JAL optimization. I think we should use 2 now everywhere. However, we must be certain that all tests work, with and without snapshots on sim and board. It might be prudent to remove all this cruft, and test/submit in a separate CL from this JIC work. https://codereview.chromium.org/1573983002/diff/1/src/mips/macro-assembler-mi... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:3106: and_(at, at, at); What's the reason for this and()? If it needs to be here to pad size or something, please add a comment.
https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:573: I would add 1 inline function to the header with this function body. Without DCHECK() or modify Instruction::InstructionType to work for Instr type and check if the instruction is a kImmediateType. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:800: DCHECK(IsBranch(instr) || IsLui(instr) || IsJicOrJialc(instr)); It looks like In the bellow code block that only instr2 can be JicOrJialc so I think instr cannot be (only a lui) so this DCHECK should not be modified, I think. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:861: DCHECK(IsBranch(instr) || IsLui(instr) || IsJicOrJialc(instr)); Like at the previous DCHECK() comment.
https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips-inl.h File src/mips/assembler-mips-inl.h (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips-inl... src/mips/assembler-mips-inl.h:375: (Assembler::IsJal(instr2) || Assembler::IsJalr(instr2))) { On 2016/01/11 22:34:57, paul.l... wrote: > I suspect that 'IsJal(instr2)' is not needed anymore after > https://codereview.chromium.org/1419793014 and should be removed. Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:573: On 2016/01/12 11:30:40, balazs.kilvady wrote: > I would add 1 inline function to the header with this function body. Without > DCHECK() or modify Instruction::InstructionType to work for Instr type and check > if the instruction is a kImmediateType. Acknowledged. There already exists a function GetImmediate16 in assembler-mips.cc, maybe I can just use that one? https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:726: uint32_t Assembler::createTargetAddress(Instr instr_lui, Instr instr_jic) { On 2016/01/11 22:34:57, paul.l... wrote: > nit: Start with capital C: CreateTargetAddress, or change to all > lower_case_with_underscore. Here and two places below. Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:735: uint32_t lui_offsetU = ((uint32_t)lui_offset) << kLuiShift; On 2016/01/11 22:34:57, paul.l... wrote: > nit: probably should not end in 'U', suggest '_u' Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:742: // Idea is to use just lui and jic instructions. We will insert lower On 2016/01/11 22:34:57, paul.l... wrote: > nit: remove 'Idea is to ', and start with 'Use just ...' > Also, please correct the word-wrapping of this comment block. There should not > be single words on a line, except possibly the last line. Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:800: DCHECK(IsBranch(instr) || IsLui(instr) || IsJicOrJialc(instr)); On 2016/01/12 11:30:40, balazs.kilvady wrote: > It looks like In the bellow code block that only instr2 can be JicOrJialc so I > think instr cannot be (only a lui) so this DCHECK should not be modified, I > think. Acknowledged. At first I thought you are referring to the DCHECK several lines below, since I see the comment below the line, but now I see what you mean. I will fix this. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:861: DCHECK(IsBranch(instr) || IsLui(instr) || IsJicOrJialc(instr)); On 2016/01/12 11:30:40, balazs.kilvady wrote: > Like at the previous DCHECK() comment. Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:872: if (IsJicOrJialc(instr2)) { On 2016/01/11 22:34:57, paul.l... wrote: > Just for readability, can this if (IsJicOrJialc(instr2)) { } be combined with > the one just below the instr2 &= ~kImm16Mask; ? It doesn't look like there are > dependencies there. Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.cc#... src/mips/assembler-mips.cc:873: uint32_t lui_offsetU, jic_offsetU; On 2016/01/11 22:34:57, paul.l... wrote: > nit: as before, don't use capital U at end like this. _u Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.h File src/mips/assembler-mips.h (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.h#n... src/mips/assembler-mips.h:526: // follows LUI/ORI pair is substituted with J/JAL, this constant equals On 2016/01/11 22:34:57, paul.l... wrote: > These comments about J/JAL should have been removed in > https://codereview.chromium.org/1419793014. Please remove them now. Done. https://codereview.chromium.org/1573983002/diff/1/src/mips/assembler-mips.h#n... src/mips/assembler-mips.h:532: static const int kInstructionsFor32BitConstant = 3; On 2016/01/11 22:34:57, paul.l... wrote: > I _think_ this case of 3 was only useful when we had J/JAL optimization. I think > we should use 2 now everywhere. However, we must be certain that all tests work, > with and without snapshots on sim and board. It might be prudent to remove all > this cruft, and test/submit in a separate CL from this JIC work. Done. New CL created. https://codereview.chromium.org/1573983002/diff/1/src/mips/macro-assembler-mi... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1573983002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:3106: and_(at, at, at); On 2016/01/11 22:34:57, paul.l... wrote: > What's the reason for this and()? If it needs to be here to pad size or > something, please add a comment. Done.
https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... File src/mips/assembler-mips-inl.h (right): https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips-inl.h:244: // Encoded internal references are lui/ori or lui/jic load of 32-bit abolute s/abolute/absolute/ https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips.cc:723: uint32_t jic_offset_u = ((uint32_t)jic_offset) & kImm16Mask; Please use static_cast<uint32_t>(*_offset) in the above lines as it is the preferred casting syntax. https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips.cc:753: jic_offset = ((uint32_t)jic_offset16) & kImm16Mask; Please use static_cast<uint32_t>(*_offset) in the above lines as it is the preferred casting syntax. https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips.cc:778: DCHECK(IsBranch(instr) || IsLui(instr) || IsJicOrJialc(instr)); It looks like from the bellow code block that only instr2 can be JicOrJialc so I think in the above DCHECK() instr cannot be (and not allowed to be) a JicOrJialc (only a lui) so this DCHECK should not be modified, I think.
https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... File src/mips/assembler-mips-inl.h (right): https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips-inl.h:244: // Encoded internal references are lui/ori or lui/jic load of 32-bit abolute On 2016/02/19 12:37:40, balazs.kilvady wrote: > s/abolute/absolute/ Done. Also fixed in few more places. https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips.cc File src/mips/assembler-mips.cc (right): https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips.cc:723: uint32_t jic_offset_u = ((uint32_t)jic_offset) & kImm16Mask; On 2016/02/19 12:37:40, balazs.kilvady wrote: > Please use static_cast<uint32_t>(*_offset) in the above lines as it is the > preferred casting syntax. Done. https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips.cc:753: jic_offset = ((uint32_t)jic_offset16) & kImm16Mask; On 2016/02/19 12:37:40, balazs.kilvady wrote: > Please use static_cast<uint32_t>(*_offset) in the above lines as it is the > preferred casting syntax. Done. https://codereview.chromium.org/1573983002/diff/40001/src/mips/assembler-mips... src/mips/assembler-mips.cc:778: DCHECK(IsBranch(instr) || IsLui(instr) || IsJicOrJialc(instr)); On 2016/02/19 12:37:40, balazs.kilvady wrote: > It looks like from the bellow code block that only instr2 can be JicOrJialc so I > think in the above DCHECK() > instr cannot be (and not allowed to be) a JicOrJialc (only a lui) so this DCHECK > should not be modified, I > think. Done.
lgtm
The CQ bit was checked by miran.karic@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1573983002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1573983002/60001
Message was sent while issue was closed.
Description was changed from ========== MIPS: Replace JR/JALR with JIC/JIALC for r6 This is the first step in process of replacing JR and JALR instructions with JIC and JIALC for r6. Trampoline in r6 now uses JIC. Also BranchLong and BranchAndLinkLong MacroAssembler functions now use JIC and JIALC in r6 if branch delay slot is not used. BUG= ========== to ========== MIPS: Replace JR/JALR with JIC/JIALC for r6 This is the first step in process of replacing JR and JALR instructions with JIC and JIALC for r6. Trampoline in r6 now uses JIC. Also BranchLong and BranchAndLinkLong MacroAssembler functions now use JIC and JIALC in r6 if branch delay slot is not used. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MIPS: Replace JR/JALR with JIC/JIALC for r6 This is the first step in process of replacing JR and JALR instructions with JIC and JIALC for r6. Trampoline in r6 now uses JIC. Also BranchLong and BranchAndLinkLong MacroAssembler functions now use JIC and JIALC in r6 if branch delay slot is not used. BUG= ========== to ========== MIPS: Replace JR/JALR with JIC/JIALC for r6 This is the first step in process of replacing JR and JALR instructions with JIC and JIALC for r6. Trampoline in r6 now uses JIC. Also BranchLong and BranchAndLinkLong MacroAssembler functions now use JIC and JIALC in r6 if branch delay slot is not used. BUG= Committed: https://crrev.com/c766f739ddd01b3688cfa94ec295b768f3edadf4 Cr-Commit-Position: refs/heads/master@{#34236} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c766f739ddd01b3688cfa94ec295b768f3edadf4 Cr-Commit-Position: refs/heads/master@{#34236} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
