Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1416493002:
Implements simple returns and call args for Mips. (Closed)
|
Created:
5 years, 2 months ago by rkotlerimgtec Modified:
5 years, 2 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionImplements simple returns and call args for Mips.
This patch is essentially the same as for ARM https://codereview.chromium.org/1127963004
I have incorporated the new 64 bit register work which was not available
at the time of this earlier patch.
The MIPS O32 Abi is not perfect on this patch but I am more or less following
the development of the ARM patches and those were preliminary at this
stage too. I will make corrections in a later patch when I incorporate
more of the ARM patches.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=ac8da5cf6f08122c15db29215a3eb137515528d9
Patch Set 1 #
Total comments: 69
Patch Set 2 : Incorporating changes from stichnot #
Total comments: 26
Patch Set 3 : Incorporation changes from Stichnot re: patch 2 #
Total comments: 5
Patch Set 4 : Corrections to patch set 3 per stichnot review #
Messages
Total messages: 10 (2 generated)
rkotlerimgtec@gmail.com changed reviewers: + jpp@google.com, stichnot@google.com
stichnot@chromium.org changed reviewers: + jpp@chromium.org, stichnot@chromium.org - jpp@google.com, stichnot@google.com
Here are some initial surface comments. Also, the Subject of the CL, and the first line of the description, should be a concise description of 80 or fewer characters, in the style of git. You can edit that here directly. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.cp... src/IceInstMIPS32.cpp:15: #include <limits> Generally our include order is: #include "Ice*.h" #include "llvm/*.h" #include <*> so move this to the end of the list. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.cp... src/IceInstMIPS32.cpp:51: void InstMIPS32::emitUnaryopGPR(const char *Opcode, const InstMIPS32 *Inst, Dump and textual emit routines usually go at the end of the file under the "=== Dump routines ===" comment. The original reason for this was the thought that we might be able to make a stripped-down, minimal, production build in part by just putting all the dump/emit routines inside an #ifdef region. Ultimately, we used the easier-to-read approach of leaving the functions but making them empty through the BuildDefs::dump() test at the beginning. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.cp... src/IceInstMIPS32.cpp:53: Ostream &Str = Func->getContext()->getStrEmit(); Add the following to the beginning of all dump() and emit() routines that do their business through Ostream output: if (!BuildDefs::dump()) return; https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.cp... src/IceInstMIPS32.cpp:54: // assert(Inst->getSrcSize() == 1); Don't leave commented-out code in the code base. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.cp... src/IceInstMIPS32.cpp:125: Str << "\n"; Usually the emit() routine doesn't print the ending newline. That is handled by the emit() caller, because e.g. under the -asm-verbose flag, it might add extra annotations before the newline as textual comments. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.de... src/IceInstMIPS32.def:35: {RegMIPS32::r0, RegMIPS32::r1, RegMIPS32::r2, RegMIPS32::r3, RegMIPS32::r4, \ For this whole file, please make sure lines don't exceed 80 columns, and make sure continuation backslash characters are consistently in column 80. Unfortunately, we can't use clang-format, since it tends to mangle whatever tabular formatting we want to impose. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.de... src/IceInstMIPS32.def:148: // isInt, isI64Pair, isFP32, isFP64, isVec128, aliases_init) change all "aliases_init" to "alias_init" for consistency with the rest of the code base. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.de... src/IceInstMIPS32.def:155: /* val, encode, name, scratch, preserved, stackptr, frameptr, isInt, isFP */ \ The comment here, and the commented-out "#define X(...)" below, should be updated with the new macro args. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:47: void emit(const Cfg *Func) const override { Remove this emit() method? https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:90: // void dump(const Cfg *Func, Ostream &Str) const override; remove commented-out code https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:102: virtual void dump(const Cfg *Func, Ostream &Str) const override { I don't think you need both "virtual" and "override". https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:113: // Variable *Index; remove https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:244: (void)Func; remove this, as Func is used below. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:287: this->Dead = Dest; I think maybe you meant to set this->Dest, not this->Dead? And if so, you don't need to initialize Dest since the Inst base class ctor already does that. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:289: this->Imm = Imm; Initialize this->Imm in the initializer list. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:294: this->Dest = Dest; Dest is already initialized in the Inst base class ctor. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:295: this->Imm = Imm; Initialize in the initializer list. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceInstMIPS32.h#... src/IceInstMIPS32.h:300: uint32_t Imm; If you initialize Imm everywhere via an initializer list, you should be able to make it a const field. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... File src/IceTargetLoweringMIPS32.cpp (right): https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:48: const uint32_t MIPS32_MAX_GPR_ARG = 4; Use constexpr if possible. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:333: UnimplementedError(Func->getContext()->getFlags()); Is the stack reference part actually unimplemented? If it's already implemented, remove this and the "return;" above. https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:339: // somewhat complex but will be implemented in it's totality through follow its https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:343: (void)NumGPRRegsUsed; remove this https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:349: for (SizeT I = 0, E = Args.size(); I < E; ++I) { I can't tell for sure whether I or E are really used in the loop, but if not, change this to a range-based for loop: for (Variable *Arg : Args) { https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:355: continue; Don't use "else if" following a "continue". I.e., if (...) { ... continue; } if (...) { // not "else if" https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:421: just one blank line here https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:427: if (ConstantInteger64 *Const = llvm::dyn_cast<ConstantInteger64>(Operand)) { auto *Const https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:430: if (OperandMIPS32Mem *Mem = llvm::dyn_cast<OperandMIPS32Mem>(Operand)) { auto *Mem https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:447: if (ConstantInteger64 *Const = llvm::dyn_cast<ConstantInteger64>(Operand)) { auto *Const https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:451: if (OperandMIPS32Mem *Mem = llvm::dyn_cast<OperandMIPS32Mem>(Operand)) { auto *Mem https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.cpp:460: const bool SignExt = false; constexpr https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... File src/IceTargetLoweringMIPS32.h (right): https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.h:106: /// marked as infinite register allocation weight, and returned Can you change "infinite register allocation weight" to "RR_MustHaveRegister"? https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.h:113: (void)C; If you don't need the value of C, change the above line to llvm::isa<ConstantRelocatable>(Src0) . https://chromiumcodereview.appspot.com/1416493002/diff/1/src/IceTargetLowerin... src/IceTargetLoweringMIPS32.h:133: /// Operand legalization helpers. To deal with address mode Reflow this comment paragraph to 80 columns. Actually do that for all the comment paragraphs here. https://chromiumcodereview.appspot.com/1416493002/diff/1/tests_lit/llvm2ice_t... File tests_lit/llvm2ice_tests/int-arg.ll (right): https://chromiumcodereview.appspot.com/1416493002/diff/1/tests_lit/llvm2ice_t... tests_lit/llvm2ice_tests/int-arg.ll:57: ; MIPS32: move v0,a1 remove tab characters
https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.cpp#newco... src/IceInstMIPS32.cpp:15: #include <limits> On 2015/10/18 11:48:38, stichnot wrote: > Generally our include order is: > #include "Ice*.h" > #include "llvm/*.h" > #include <*> > > so move this to the end of the list. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.cpp#newco... src/IceInstMIPS32.cpp:51: void InstMIPS32::emitUnaryopGPR(const char *Opcode, const InstMIPS32 *Inst, On 2015/10/18 11:48:37, stichnot wrote: > Dump and textual emit routines usually go at the end of the file under the "=== > Dump routines ===" comment. > > The original reason for this was the thought that we might be able to make a > stripped-down, minimal, production build in part by just putting all the > dump/emit routines inside an #ifdef region. Ultimately, we used the > easier-to-read approach of leaving the functions but making them empty through > the BuildDefs::dump() test at the beginning. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.cpp#newco... src/IceInstMIPS32.cpp:53: Ostream &Str = Func->getContext()->getStrEmit(); On 2015/10/18 11:48:37, stichnot wrote: > Add the following to the beginning of all dump() and emit() routines that do > their business through Ostream output: > > if (!BuildDefs::dump()) > return; Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.cpp#newco... src/IceInstMIPS32.cpp:54: // assert(Inst->getSrcSize() == 1); On 2015/10/18 11:48:37, stichnot wrote: > Don't leave commented-out code in the code base. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.cpp#newco... src/IceInstMIPS32.cpp:125: Str << "\n"; On 2015/10/18 11:48:38, stichnot wrote: > Usually the emit() routine doesn't print the ending newline. That is handled by > the emit() caller, because e.g. under the -asm-verbose flag, it might add extra > annotations before the newline as textual comments. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:35: {RegMIPS32::r0, RegMIPS32::r1, RegMIPS32::r2, RegMIPS32::r3, RegMIPS32::r4, \ On 2015/10/18 11:48:38, stichnot wrote: > For this whole file, please make sure lines don't exceed 80 columns, and make > sure continuation backslash characters are consistently in column 80. > > Unfortunately, we can't use clang-format, since it tends to mangle whatever > tabular formatting we want to impose. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:148: // isInt, isI64Pair, isFP32, isFP64, isVec128, aliases_init) On 2015/10/18 11:48:38, stichnot wrote: > change all "aliases_init" to "alias_init" for consistency with the rest of the > code base. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.def#newco... src/IceInstMIPS32.def:155: /* val, encode, name, scratch, preserved, stackptr, frameptr, isInt, isFP */ \ On 2015/10/18 11:48:38, stichnot wrote: > The comment here, and the commented-out "#define X(...)" below, should be > updated with the new macro args. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode47 src/IceInstMIPS32.h:47: void emit(const Cfg *Func) const override { On 2015/10/18 11:48:39, stichnot wrote: > Remove this emit() method? Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode90 src/IceInstMIPS32.h:90: // void dump(const Cfg *Func, Ostream &Str) const override; On 2015/10/18 11:48:39, stichnot wrote: > remove commented-out code Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode102 src/IceInstMIPS32.h:102: virtual void dump(const Cfg *Func, Ostream &Str) const override { On 2015/10/18 11:48:39, stichnot wrote: > I don't think you need both "virtual" and "override". Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode113 src/IceInstMIPS32.h:113: // Variable *Index; On 2015/10/18 11:48:39, stichnot wrote: > remove Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode244 src/IceInstMIPS32.h:244: (void)Func; On 2015/10/18 11:48:39, stichnot wrote: > remove this, as Func is used below. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode287 src/IceInstMIPS32.h:287: this->Dead = Dest; On 2015/10/18 11:48:39, stichnot wrote: > I think maybe you meant to set this->Dest, not this->Dead? > > And if so, you don't need to initialize Dest since the Inst base class ctor > already does that. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode289 src/IceInstMIPS32.h:289: this->Imm = Imm; On 2015/10/18 11:48:38, stichnot wrote: > Initialize this->Imm in the initializer list. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode294 src/IceInstMIPS32.h:294: this->Dest = Dest; On 2015/10/18 11:48:38, stichnot wrote: > Dest is already initialized in the Inst base class ctor. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode295 src/IceInstMIPS32.h:295: this->Imm = Imm; On 2015/10/18 11:48:38, stichnot wrote: > Initialize in the initializer list. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceInstMIPS32.h#newcode300 src/IceInstMIPS32.h:300: uint32_t Imm; On 2015/10/18 11:48:38, stichnot wrote: > If you initialize Imm everywhere via an initializer list, you should be able to > make it a const field. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:48: const uint32_t MIPS32_MAX_GPR_ARG = 4; On 2015/10/18 11:48:40, stichnot wrote: > Use constexpr if possible. Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:333: UnimplementedError(Func->getContext()->getFlags()); On 2015/10/18 11:48:39, stichnot wrote: > Is the stack reference part actually unimplemented? If it's already > implemented, remove this and the "return;" above. It's incomplete. I was starting to add it but realized that it's part of a later ARM patch and I'm trying to keep these two ports in step. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:339: // somewhat complex but will be implemented in it's totality through follow On 2015/10/18 11:48:39, stichnot wrote: > its Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:343: (void)NumGPRRegsUsed; On 2015/10/18 11:48:39, stichnot wrote: > remove this Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:349: for (SizeT I = 0, E = Args.size(); I < E; ++I) { On 2015/10/18 11:48:40, stichnot wrote: > I can't tell for sure whether I or E are really used in the loop, but if not, > change this to a range-based for loop: > > for (Variable *Arg : Args) { I is used. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:355: continue; On 2015/10/18 11:48:39, stichnot wrote: > Don't use "else if" following a "continue". I.e., > > if (...) { > ... > continue; > } > if (...) { // not "else if" Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:421: On 2015/10/18 11:48:39, stichnot wrote: > just one blank line here Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:427: if (ConstantInteger64 *Const = llvm::dyn_cast<ConstantInteger64>(Operand)) { On 2015/10/18 11:48:39, stichnot wrote: > auto *Const Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:427: if (ConstantInteger64 *Const = llvm::dyn_cast<ConstantInteger64>(Operand)) { On 2015/10/18 11:48:39, stichnot wrote: > auto *Const Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:430: if (OperandMIPS32Mem *Mem = llvm::dyn_cast<OperandMIPS32Mem>(Operand)) { On 2015/10/18 11:48:39, stichnot wrote: > auto *Mem Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:447: if (ConstantInteger64 *Const = llvm::dyn_cast<ConstantInteger64>(Operand)) { On 2015/10/18 11:48:40, stichnot wrote: > auto *Const Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:451: if (OperandMIPS32Mem *Mem = llvm::dyn_cast<OperandMIPS32Mem>(Operand)) { On 2015/10/18 11:48:39, stichnot wrote: > auto *Mem Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:460: const bool SignExt = false; On 2015/10/18 11:48:39, stichnot wrote: > constexpr Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32.h File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.h:106: /// marked as infinite register allocation weight, and returned On 2015/10/18 11:48:40, stichnot wrote: > Can you change "infinite register allocation weight" to "RR_MustHaveRegister"? Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.h:113: (void)C; On 2015/10/18 11:48:40, stichnot wrote: > If you don't need the value of C, change the above line to > llvm::isa<ConstantRelocatable>(Src0) . Done. https://codereview.chromium.org/1416493002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.h:133: /// Operand legalization helpers. To deal with address mode On 2015/10/18 11:48:40, stichnot wrote: > Reflow this comment paragraph to 80 columns. > > Actually do that for all the comment paragraphs here. Done. https://codereview.chromium.org/1416493002/diff/1/tests_lit/llvm2ice_tests/in... File tests_lit/llvm2ice_tests/int-arg.ll (right): https://codereview.chromium.org/1416493002/diff/1/tests_lit/llvm2ice_tests/in... tests_lit/llvm2ice_tests/int-arg.ll:57: ; MIPS32: move v0,a1 On 2015/10/18 11:48:40, stichnot wrote: > remove tab characters Done.
https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... File src/IceInstMIPS32.cpp (right): https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.cpp:239: } add the "end of namespace Ice" comment https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... File src/IceInstMIPS32.def (right): https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.def:35: {RegMIPS32::r0, RegMIPS32::r1, RegMIPS32::r2, RegMIPS32::r3, RegMIPS32::r4, \ 80-col https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.def:42: isInt, isI64Pair, isFP32, isFP64, isVec128, alias_init */ \ fix trailing backslash alignment on lines 42, 46, 53, 122. https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.def:108: // isInt, isFP) This line is out of date, copy it from line 148 I think. https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.def:161: // isInt, isFP) Also replace this with the version from line 148, I think https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.h:207: : InstMIPS32(Func, K, 2, Dest) { I think the "2" should be a "1". This argument tells how large to pre-allocate the Srcs[] array, and should be equal to the number of addSource() calls. Allocating too large an array doesn't affect correctness, but it may have a small impact on translation speed. https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.h:275: : InstMIPS32(Func, K, 2, Dest), Imm(Imm){ As above, the "2" should be a "1". https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceInstMIPS3... src/IceInstMIPS32.h:280: : InstMIPS32(Func, K, 1, Dest), Imm(Imm) { Similarly, the "1" would be "0". https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceRegisters... File src/IceRegistersMIPS32.h (right): https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceRegisters... src/IceRegistersMIPS32.h:31: \ Remove extra blank line for consistency with other uses of x-macros. Here and below. (That said, it would be nice to find a more pleasing style of x-macro blank line usage to use everywhere.) https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceTargetLow... File src/IceTargetLoweringMIPS32.cpp (right): https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceTargetLow... src/IceTargetLoweringMIPS32.cpp:1048: /// Provide a trivial wrapper to legalize() for this common usage. just remove all this https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceTargetLow... File src/IceTargetLoweringMIPS32.h (right): https://chromiumcodereview.appspot.com/1416493002/diff/20001/src/IceTargetLow... src/IceTargetLoweringMIPS32.h:106: /// If Dest=nullptr is passed in, then a new variable is created, marked as Since you're following the ARM development, you should probably remove this comment because it's incorrect (outdated). Similarly, remove the commented-out RegNum arg. Also, remove one of the two blank lines above. https://chromiumcodereview.appspot.com/1416493002/diff/20001/tests_lit/llvm2i... File tests_lit/llvm2ice_tests/int-arg.ll (right): https://chromiumcodereview.appspot.com/1416493002/diff/20001/tests_lit/llvm2i... tests_lit/llvm2ice_tests/int-arg.ll:44: ; MIPS32-LABEL: <test_returning32_arg0> What do you think about using MIPS32-LABEL: test_returning32_arg0 instead of MIPS32-LABEL: <test_returning32_arg0> for consistency? (here and below)
https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:239: } On 2015/10/20 04:40:59, stichnot wrote: > add the "end of namespace Ice" comment Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:35: {RegMIPS32::r0, RegMIPS32::r1, RegMIPS32::r2, RegMIPS32::r3, RegMIPS32::r4, \ On 2015/10/20 04:40:59, stichnot wrote: > 80-col Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:42: isInt, isI64Pair, isFP32, isFP64, isVec128, alias_init */ \ On 2015/10/20 04:40:59, stichnot wrote: > fix trailing backslash alignment on lines 42, 46, 53, 122. Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:108: // isInt, isFP) On 2015/10/20 04:40:59, stichnot wrote: > This line is out of date, copy it from line 148 I think. Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:161: // isInt, isFP) On 2015/10/20 04:40:59, stichnot wrote: > Also replace this with the version from line 148, I think Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:207: : InstMIPS32(Func, K, 2, Dest) { On 2015/10/20 04:41:00, stichnot wrote: > I think the "2" should be a "1". > > This argument tells how large to pre-allocate the Srcs[] array, and should be > equal to the number of addSource() calls. > > Allocating too large an array doesn't affect correctness, but it may have a > small impact on translation speed. Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:275: : InstMIPS32(Func, K, 2, Dest), Imm(Imm){ On 2015/10/20 04:41:00, stichnot wrote: > As above, the "2" should be a "1". Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:280: : InstMIPS32(Func, K, 1, Dest), Imm(Imm) { On 2015/10/20 04:41:00, stichnot wrote: > Similarly, the "1" would be "0". Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceRegistersMIPS32.h File src/IceRegistersMIPS32.h (right): https://codereview.chromium.org/1416493002/diff/20001/src/IceRegistersMIPS32.... src/IceRegistersMIPS32.h:31: \ On 2015/10/20 04:41:00, stichnot wrote: > Remove extra blank line for consistency with other uses of x-macros. > Here and below. > > (That said, it would be nice to find a more pleasing style of x-macro blank line > usage to use everywhere.) Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1416493002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1048: /// Provide a trivial wrapper to legalize() for this common usage. On 2015/10/20 04:41:00, stichnot wrote: > just remove all this Done. https://codereview.chromium.org/1416493002/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1416493002/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.h:106: /// If Dest=nullptr is passed in, then a new variable is created, marked as On 2015/10/20 04:41:00, stichnot wrote: > Since you're following the ARM development, you should probably remove this > comment because it's incorrect (outdated). Similarly, remove the commented-out > RegNum arg. Also, remove one of the two blank lines above. Done. https://codereview.chromium.org/1416493002/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/int-arg.ll (right): https://codereview.chromium.org/1416493002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/int-arg.ll:44: ; MIPS32-LABEL: <test_returning32_arg0> On 2015/10/20 04:41:00, stichnot wrote: > What do you think about using > MIPS32-LABEL: test_returning32_arg0 > instead of > MIPS32-LABEL: <test_returning32_arg0> > for consistency? (here and below) For some reason I was having trouble with make check when i did not have the <> . I can investigate and make the change on a follow-up patch if that's okay or else we can try and resolve this now.
Otherwise LGTM, thanks! After the updated patchset, I should be able to land it myself. https://codereview.chromium.org/1416493002/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/int-arg.ll (right): https://codereview.chromium.org/1416493002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/int-arg.ll:44: ; MIPS32-LABEL: <test_returning32_arg0> On 2015/10/21 00:30:54, rkotlerimgtec wrote: > On 2015/10/20 04:41:00, stichnot wrote: > > What do you think about using > > MIPS32-LABEL: test_returning32_arg0 > > instead of > > MIPS32-LABEL: <test_returning32_arg0> > > for consistency? (here and below) > > For some reason I was having trouble with make check when i did > not have the <> . I can investigate and make the change on > a follow-up patch if that's okay or else we can try and resolve this > now. I just gave it a try locally (just in this file), and it gave me no trouble. So maybe just get it over with in this CL? https://codereview.chromium.org/1416493002/diff/40001/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1416493002/diff/40001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:52: X(Reg_A1, = Reg_ZERO + 5, "a1", 1, 0, 0, 0, 1, 0, 0, 0, 0, \ 80-col https://codereview.chromium.org/1416493002/diff/40001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:152: #define REGMIPS32_TABLE \ put the trailing underscore at column 80
I added a fix to return_immediates to remove <> there too. Not sure what the original problem I was having was from that caused me to add the <> for labels. https://codereview.chromium.org/1416493002/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/int-arg.ll (right): https://codereview.chromium.org/1416493002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/int-arg.ll:44: ; MIPS32-LABEL: <test_returning32_arg0> On 2015/10/21 02:12:17, stichnot wrote: > On 2015/10/21 00:30:54, rkotlerimgtec wrote: > > On 2015/10/20 04:41:00, stichnot wrote: > > > What do you think about using > > > MIPS32-LABEL: test_returning32_arg0 > > > instead of > > > MIPS32-LABEL: <test_returning32_arg0> > > > for consistency? (here and below) > > > > For some reason I was having trouble with make check when i did > > not have the <> . I can investigate and make the change on > > a follow-up patch if that's okay or else we can try and resolve this > > now. > > I just gave it a try locally (just in this file), and it gave me no trouble. So > maybe just get it over with in this CL? Done. https://codereview.chromium.org/1416493002/diff/40001/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1416493002/diff/40001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:52: X(Reg_A1, = Reg_ZERO + 5, "a1", 1, 0, 0, 0, 1, 0, 0, 0, 0, \ On 2015/10/21 02:12:17, stichnot wrote: > 80-col Done. https://codereview.chromium.org/1416493002/diff/40001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:152: #define REGMIPS32_TABLE \ On 2015/10/21 02:12:17, stichnot wrote: > put the trailing underscore at column 80 Done. https://codereview.chromium.org/1416493002/diff/40001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:152: #define REGMIPS32_TABLE \ On 2015/10/21 02:12:17, stichnot wrote: > put the trailing underscore at column 80 Done.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as ac8da5cf6f08122c15db29215a3eb137515528d9 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
