Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1176133004:
implement the null function for the Mips32 subzero compiler (Closed)
|
Created:
5 years, 6 months ago by reed.kotler Modified:
5 years, 5 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. |
Descriptionimplement the null function for the Mips32 subzero compiler
BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167
R=jvoung@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=d00d48da12ac44879066724978a032be64fcb0ad
Patch Set 1 #Patch Set 2 : Run clang format #
Total comments: 16
Patch Set 3 : Fix patch to synch to master #
Total comments: 59
Patch Set 4 : Address code review comments #
Total comments: 5
Patch Set 5 : git diff #Patch Set 6 : Fixing patch per review comments #
Total comments: 1
Messages
Total messages: 20 (1 generated)
reed.kotler@imgtec.com changed reviewers: + jvoung@chromium.org, stichnot@chromium.org
Implement the null function in Mips subzero. When running make Makefile.standalone format, it changed a few lines in non Mips files.
Reed, BTW, do you have a link to some good public reference for the MIPS instruction set, to help us with code reviews? https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:1: //===- subzero/src/IceInstARM32.cpp - ARM32 instruction implementation ----===// MIPS https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:10: // This file implements the InstARM32 and OperandARM32 classes, MIPS https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:26: namespace {} remove this? https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:62: } // end of namespace Ice (maybe an extra blank line between the two "}" lines) https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:359: ; stray ; https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:391: ; stray ; https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; remove this? https://codereview.chromium.org/1176133004/diff/20001/src/IceTypes.def File src/IceTypes.def (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceTypes.def#newcode29 src/IceTypes.def:29: X(Target_MIPS32, "mips32", false, EM_MIPS, 0) \ fix column alignment, and also make sure the \ column lines up with the rest of the file
https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h File src/IceAssemblerMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.... src/IceAssemblerMIPS32.h:53: // TBD (Reed Kotler ) . Find out what this should be. nit: Usually we try to format the "TODO"s consistently as TODO(username), so that it's easier to grep for. E.g., you have TODO(reed kotler) in some other places. https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:28: InstMIPS32Ret::InstMIPS32Ret(Cfg *Func, Variable *LR, Variable *Source) s/LR/RA/ https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:39: Variable *LR = llvm::cast<Variable>(getSrc(0)); s/LR/RA/ https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:55: // (TODO Reed Kotler) TODO(reed kotler) https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:63: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, s/LR/RA/ https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.h:74: void _ret(Variable *LR, Variable *Src0 = nullptr) { LR -> RA https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringX8... File src/IceTargetLoweringX8632.cpp (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringX8... src/IceTargetLoweringX8632.cpp:3122: lowerAtomicRMW( btw, what version of clang-format do you have? Is it from LLVM 3.6 or 3.7? E.g., for me: ~/nacl/native_client/toolchain/linux_x86/pnacl_newlib_raw/bin/clang-format --version clang-format version 3.7.0 ... https://codereview.chromium.org/1176133004/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:42: ; TOOD (Reed Kotler check this disassembly, .s file looks fine) TODO(reed kotler)
On 2015/06/12 19:39:13, jvoung wrote: > https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h > File src/IceAssemblerMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.... > src/IceAssemblerMIPS32.h:53: // TBD (Reed Kotler ) . Find out what this should > be. > nit: Usually we try to format the "TODO"s consistently as TODO(username), so > that it's easier to grep for. > > E.g., you have TODO(reed kotler) in some other places. > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp > File src/IceInstMIPS32.cpp (right): > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:28: InstMIPS32Ret::InstMIPS32Ret(Cfg *Func, Variable *LR, > Variable *Source) > s/LR/RA/ > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:39: Variable *LR = llvm::cast<Variable>(getSrc(0)); > s/LR/RA/ > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h > File src/IceInstMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:55: // (TODO Reed Kotler) > TODO(reed kotler) > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:63: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, > s/LR/RA/ > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... > File src/IceTargetLoweringMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.h:74: void _ret(Variable *LR, Variable *Src0 = > nullptr) { > LR -> RA > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringX8... > File src/IceTargetLoweringX8632.cpp (right): > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringX8... > src/IceTargetLoweringX8632.cpp:3122: lowerAtomicRMW( > btw, what version of clang-format do you have? Is it from LLVM 3.6 or 3.7? E.g., > for me: > > ~/nacl/native_client/toolchain/linux_x86/pnacl_newlib_raw/bin/clang-format > --version > clang-format version 3.7.0 ... > > https://codereview.chromium.org/1176133004/diff/20001/tests_lit/llvm2ice_test... > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > https://codereview.chromium.org/1176133004/diff/20001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:42: ; TOOD (Reed Kotler check this > disassembly, .s file looks fine) > TODO(reed kotler) Thanks a lot for getting to this patch so quickly. I'm going away for 3 weeks starting tomorrow so I don't know if I will finish all the corrections and testing today since I have some things to do before leaving. I will try and fix the patch while I'm away and resubmit it.
On 2015/06/12 20:01:33, reed.kotler wrote: > On 2015/06/12 19:39:13, jvoung wrote: > > https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h > > File src/IceAssemblerMIPS32.h (right): > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.... > > src/IceAssemblerMIPS32.h:53: // TBD (Reed Kotler ) . Find out what this should > > be. > > nit: Usually we try to format the "TODO"s consistently as TODO(username), so > > that it's easier to grep for. > > > > E.g., you have TODO(reed kotler) in some other places. > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp > > File src/IceInstMIPS32.cpp (right): > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:28: InstMIPS32Ret::InstMIPS32Ret(Cfg *Func, Variable > *LR, > > Variable *Source) > > s/LR/RA/ > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:39: Variable *LR = llvm::cast<Variable>(getSrc(0)); > > s/LR/RA/ > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h > > File src/IceInstMIPS32.h (right): > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h#new... > > src/IceInstMIPS32.h:55: // (TODO Reed Kotler) > > TODO(reed kotler) > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceInstMIPS32.h#new... > > src/IceInstMIPS32.h:63: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, > > s/LR/RA/ > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... > > File src/IceTargetLoweringMIPS32.h (right): > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringMI... > > src/IceTargetLoweringMIPS32.h:74: void _ret(Variable *LR, Variable *Src0 = > > nullptr) { > > LR -> RA > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringX8... > > File src/IceTargetLoweringX8632.cpp (right): > > > > > https://codereview.chromium.org/1176133004/diff/20001/src/IceTargetLoweringX8... > > src/IceTargetLoweringX8632.cpp:3122: lowerAtomicRMW( > > btw, what version of clang-format do you have? Is it from LLVM 3.6 or 3.7? > E.g., > > for me: > > > > ~/nacl/native_client/toolchain/linux_x86/pnacl_newlib_raw/bin/clang-format > > --version > > clang-format version 3.7.0 ... > > > > > https://codereview.chromium.org/1176133004/diff/20001/tests_lit/llvm2ice_test... > > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > > > > https://codereview.chromium.org/1176133004/diff/20001/tests_lit/llvm2ice_test... > > tests_lit/llvm2ice_tests/function_aligned.ll:42: ; TOOD (Reed Kotler check > this > > disassembly, .s file looks fine) > > TODO(reed kotler) > > Thanks a lot for getting to this patch so quickly. > > I'm going away for 3 weeks starting tomorrow so I don't know if I will finish > all the corrections and testing today since I have some things to do before > leaving. > > I will try and fix the patch while I'm away and resubmit it. Have updated the patch so that it works with the updated master branch.
On 2015/06/12 18:03:14, stichnot wrote: > Reed, BTW, do you have a link to some good public reference for the MIPS > instruction set, to help us with code reviews? > http://imgtec.com/documentation/ You can get all manuals through this link. Some high level things: Mips32, Mips64 r5/r6 Mips32 and Mips64 are mostly the same except for the larger register sizes. r5/r6 have some big differences though mostly in the micro architecture but in other areas too. lots of older instructions that are not really used anymore have been deleted and in this way the modern llvm Mips compilers for example have never used these. the accumulator for multiply/divide has been replaced with a more traditional mechanism that uses only general purpose registers. some pc relative branching has been added and other branch changes have been made.
Welcome back When review comments are addressed, you should be able to click on them in the web UI. That will pop up a text box with "Done" "Acknowledged", etc. buttons along the bottom of that text box. Click "Done" when you've addressed them so that the reviewer can check what's been addressed and what hasn't. Thanks! https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py File pydir/run-pnacl-sz.py (right): https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#n... pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] } Is it possible to leave out the "-none-nacl" for now, to be consistent with the rest, or does that break the tests (if so, what does does the error look like)? https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.h File src/IceAssemblerMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.... src/IceAssemblerMIPS32.h:55: // TBD (Reed Kotler ) . Find out what this should be. Please use a more consistent TODO marker for greppability. "TODO(reed kotler)" https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:1: //===- subzero/src/IceInstARM32.cpp - ARM32 instruction implementation ----===// ARM32 -> MIPS32 in two places https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:10: // This file implements the InstARM32 and OperandARM32 classes, MIPS32 Also, for doxygen do: """ //===--- /// /// /file /// This file implements the InstMIPS32 and ... /// //===---- """ https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:26: namespace {} Jim had a comment about removing this stray namespace {} https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:28: InstMIPS32Ret::InstMIPS32Ret(Cfg *Func, Variable *LR, Variable *Source) LR -> RA https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:41: assert(LR->getRegNum() == RegMIPS32::Reg_RA); LR -> RA https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:44: << "jr $ra" no need to print "$ra" here as part of "jr $ra" -- you have RA->emit(Func) below, which should print the $ra https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:28: // Base class for Mips instructions. Newline between "class TargetMIPS32" and the start of the classes for insts. Also, we started to doxygenize some comments. Could you convert the "//" which you think should appear in generated documentation into "///"? https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:56: // (TODO Reed Kotler) Please use a more consistent TODO marker: "TODO(reed kotler)" https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:64: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, LR -> RA in various places here and in the .cpp https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:262: UnimplementedError(Func->getContext()->getFlags()); Oh I see... previous comment about printing "jr $ra", you'll need to implement this method to get that RA->emit(Func) to do the work. Is that possible within this CL, or ? https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:359: ; you have an extra ";" here that can be removed https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:391: ; extra ";" https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; No need to do (void)Reg. You use it below in _ret(..., Reg); https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.h:79: void _ret(Variable *LR, Variable *Src0 = nullptr) { RA https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:16: ; RUN: %if --need=target_MIPS32 --command %p2i --filetype=asm --assemble \ Can you add the "--need=allow_dump " that is now part of the ARM variant? I had left that out previously. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:18: ; RUN: | %if --need=target_MIPS32 --command FileCheck --check-prefix MIPS32 %s this line also requires a "--need=allow_dump" before the --command FileCheck https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:31: ; TODO (Reed Kotler check this disassembly, .s file looks fine) "TODO(reed kotler)" for consistency Or just leave out the TODO if it looks fine? https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:35: ; MIPS32-NEXT: c: e7fedef0 I think the main TODO is that "e7fedef0" is an "undefined"/halting instruction for ARM (what llvm.trap would translate to). https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:44: ; TOOD (Reed Kotler check this disassembly, .s file looks fine) Probably don't need a TODO, since this block doesn't have the iffy "e7fedef0" check.
On 2015/07/07 18:02:35, jvoung wrote: > Welcome back > > When review comments are addressed, you should be able to click on them in the > web UI. That will pop up a text box with "Done" "Acknowledged", etc. buttons > along the bottom of that text box. Click "Done" when you've addressed them so > that the reviewer can check what's been addressed and what hasn't. > > Thanks! > > https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py > File pydir/run-pnacl-sz.py (right): > > https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#n... > pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] } > Is it possible to leave out the "-none-nacl" for now, to be consistent with the > rest, or does that break the tests (if so, what does does the error look like)? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.h > File src/IceAssemblerMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.... > src/IceAssemblerMIPS32.h:55: // TBD (Reed Kotler ) . Find out what this should > be. > Please use a more consistent TODO marker for greppability. > > "TODO(reed kotler)" > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp > File src/IceInstMIPS32.cpp (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:1: //===- subzero/src/IceInstARM32.cpp - ARM32 instruction > implementation ----===// > ARM32 -> MIPS32 in two places > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:10: // This file implements the InstARM32 and OperandARM32 > classes, > MIPS32 > > Also, for doxygen do: > > """ > //===--- > /// > /// /file > /// This file implements the InstMIPS32 and ... > /// > //===---- > """ > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:26: namespace {} > Jim had a comment about removing this stray namespace {} > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:28: InstMIPS32Ret::InstMIPS32Ret(Cfg *Func, Variable *LR, > Variable *Source) > LR -> RA > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:41: assert(LR->getRegNum() == RegMIPS32::Reg_RA); > LR -> RA > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:44: << "jr $ra" > no need to print "$ra" here as part of "jr $ra" -- you have RA->emit(Func) > below, which should print the $ra > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h > File src/IceInstMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:28: // Base class for Mips instructions. > Newline between "class TargetMIPS32" and the start of the classes for insts. > > Also, we started to doxygenize some comments. Could you convert the "//" which > you think should appear in generated documentation into "///"? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:56: // (TODO Reed Kotler) > Please use a more consistent TODO marker: > > "TODO(reed kotler)" > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:64: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, > LR -> RA in various places here and in the .cpp > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > File src/IceTargetLoweringMIPS32.cpp (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:262: > UnimplementedError(Func->getContext()->getFlags()); > Oh I see... previous comment about printing "jr $ra", you'll need to implement > this method to get that RA->emit(Func) to do the work. Is that possible within > this CL, or ? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:359: ; > you have an extra ";" here that can be removed > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:391: ; > extra ";" > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; > No need to do (void)Reg. You use it below in _ret(..., Reg); > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > File src/IceTargetLoweringMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.h:79: void _ret(Variable *LR, Variable *Src0 = > nullptr) { > RA > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:16: ; RUN: %if --need=target_MIPS32 > --command %p2i --filetype=asm --assemble \ > Can you add the "--need=allow_dump " that is now part of the ARM variant? I had > left that out previously. > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:18: ; RUN: | %if > --need=target_MIPS32 --command FileCheck --check-prefix MIPS32 %s > this line also requires a "--need=allow_dump" before the --command FileCheck > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:31: ; TODO (Reed Kotler check this > disassembly, .s file looks fine) > "TODO(reed kotler)" for consistency > > Or just leave out the TODO if it looks fine? > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:35: ; MIPS32-NEXT: c: e7fedef0 > I think the main TODO is that "e7fedef0" is an "undefined"/halting instruction > for ARM (what llvm.trap would translate to). > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:44: ; TOOD (Reed Kotler check this > disassembly, .s file looks fine) > Probably don't need a TODO, since this block doesn't have the iffy "e7fedef0" > check. I think I've addressed all issues. A few things you might still want me to change. I've replied to a few of the comments without making any code changes there but you might not agree with my point of view there. Thanks for looking at this.
On 2015/07/07 20:45:25, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > Welcome back > > > > When review comments are addressed, you should be able to click on them in the > > web UI. That will pop up a text box with "Done" "Acknowledged", etc. buttons > > along the bottom of that text box. Click "Done" when you've addressed them so > > that the reviewer can check what's been addressed and what hasn't. > > > > Thanks! > > > > https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py > > File pydir/run-pnacl-sz.py (right): > > > > > https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#n... > > pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] } > > Is it possible to leave out the "-none-nacl" for now, to be consistent with > the > > rest, or does that break the tests (if so, what does does the error look > like)? > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.h > > File src/IceAssemblerMIPS32.h (right): > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.... > > src/IceAssemblerMIPS32.h:55: // TBD (Reed Kotler ) . Find out what this should > > be. > > Please use a more consistent TODO marker for greppability. > > > > "TODO(reed kotler)" > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp > > File src/IceInstMIPS32.cpp (right): > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:1: //===- subzero/src/IceInstARM32.cpp - ARM32 > instruction > > implementation ----===// > > ARM32 -> MIPS32 in two places > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:10: // This file implements the InstARM32 and > OperandARM32 > > classes, > > MIPS32 > > > > Also, for doxygen do: > > > > """ > > //===--- > > /// > > /// /file > > /// This file implements the InstMIPS32 and ... > > /// > > //===---- > > """ > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:26: namespace {} > > Jim had a comment about removing this stray namespace {} > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:28: InstMIPS32Ret::InstMIPS32Ret(Cfg *Func, Variable > *LR, > > Variable *Source) > > LR -> RA > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:41: assert(LR->getRegNum() == RegMIPS32::Reg_RA); > > LR -> RA > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > > src/IceInstMIPS32.cpp:44: << "jr $ra" > > no need to print "$ra" here as part of "jr $ra" -- you have RA->emit(Func) > > below, which should print the $ra > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h > > File src/IceInstMIPS32.h (right): > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > > src/IceInstMIPS32.h:28: // Base class for Mips instructions. > > Newline between "class TargetMIPS32" and the start of the classes for insts. > > > > Also, we started to doxygenize some comments. Could you convert the "//" which > > you think should appear in generated documentation into "///"? > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > > src/IceInstMIPS32.h:56: // (TODO Reed Kotler) > > Please use a more consistent TODO marker: > > > > "TODO(reed kotler)" > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > > src/IceInstMIPS32.h:64: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, > > LR -> RA in various places here and in the .cpp > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > > File src/IceTargetLoweringMIPS32.cpp (right): > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > > src/IceTargetLoweringMIPS32.cpp:262: > > UnimplementedError(Func->getContext()->getFlags()); > > Oh I see... previous comment about printing "jr $ra", you'll need to implement > > this method to get that RA->emit(Func) to do the work. Is that possible within > > this CL, or ? > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > > src/IceTargetLoweringMIPS32.cpp:359: ; > > you have an extra ";" here that can be removed > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > > src/IceTargetLoweringMIPS32.cpp:391: ; > > extra ";" > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > > src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; > > No need to do (void)Reg. You use it below in _ret(..., Reg); > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > > File src/IceTargetLoweringMIPS32.h (right): > > > > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > > src/IceTargetLoweringMIPS32.h:79: void _ret(Variable *LR, Variable *Src0 = > > nullptr) { > > RA > > > > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > > > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > > tests_lit/llvm2ice_tests/function_aligned.ll:16: ; RUN: %if > --need=target_MIPS32 > > --command %p2i --filetype=asm --assemble \ > > Can you add the "--need=allow_dump " that is now part of the ARM variant? I > had > > left that out previously. > > > > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > > tests_lit/llvm2ice_tests/function_aligned.ll:18: ; RUN: | %if > > --need=target_MIPS32 --command FileCheck --check-prefix MIPS32 %s > > this line also requires a "--need=allow_dump" before the --command FileCheck > > > > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > > tests_lit/llvm2ice_tests/function_aligned.ll:31: ; TODO (Reed Kotler check > this > > disassembly, .s file looks fine) > > "TODO(reed kotler)" for consistency > > > > Or just leave out the TODO if it looks fine? > > > > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > > tests_lit/llvm2ice_tests/function_aligned.ll:35: ; MIPS32-NEXT: c: e7fedef0 > > I think the main TODO is that "e7fedef0" is an "undefined"/halting instruction > > for ARM (what llvm.trap would translate to). > > > > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > > tests_lit/llvm2ice_tests/function_aligned.ll:44: ; TOOD (Reed Kotler check > this > > disassembly, .s file looks fine) > > Probably don't need a TODO, since this block doesn't have the iffy "e7fedef0" > > check. > > I think I've addressed all issues. > > A few things you might still want me to change. I've replied to a few of the > comments without > making any code changes there but you might not agree with my point of view > there. > > Thanks for looking at this. I don't see the replies to the comments. Did you hit "Publish+Mail Comments" ?
Forgot to publish the comments. Lol https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py File pydir/run-pnacl-sz.py (right): https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#n... pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] } On 2015/07/07 18:02:34, jvoung wrote: > Is it possible to leave out the "-none-nacl" for now, to be consistent with the > rest, or does that break the tests (if so, what does does the error look like)? for now it's needed but I will try and eliminate it in the future. it will mess up the alignment in the assembler for nacl without it. https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.h File src/IceAssemblerMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceAssemblerMIPS32.... src/IceAssemblerMIPS32.h:55: // TBD (Reed Kotler ) . Find out what this should be. On 2015/07/07 18:02:34, jvoung wrote: > Please use a more consistent TODO marker for greppability. > > "TODO(reed kotler)" Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:1: //===- subzero/src/IceInstARM32.cpp - ARM32 instruction implementation ----===// On 2015/07/07 18:02:35, jvoung wrote: > ARM32 -> MIPS32 in two places Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:10: // This file implements the InstARM32 and OperandARM32 classes, On 2015/07/07 18:02:34, jvoung wrote: > MIPS32 > > Also, for doxygen do: > > """ > //===--- > /// > /// /file > /// This file implements the InstMIPS32 and ... > /// > //===---- > """ Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:26: namespace {} On 2015/07/07 18:02:34, jvoung wrote: > Jim had a comment about removing this stray namespace {} Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:28: InstMIPS32Ret::InstMIPS32Ret(Cfg *Func, Variable *LR, Variable *Source) On 2015/07/07 18:02:34, jvoung wrote: > LR -> RA Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:41: assert(LR->getRegNum() == RegMIPS32::Reg_RA); On 2015/07/07 18:02:34, jvoung wrote: > LR -> RA Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:44: << "jr $ra" On 2015/07/07 18:02:34, jvoung wrote: > no need to print "$ra" here as part of "jr $ra" -- you have RA->emit(Func) > below, which should print the $ra This is needed for now because emitVariable is not implemented yet. I will remove it when I flesh out that function. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:28: // Base class for Mips instructions. On 2015/07/07 18:02:35, jvoung wrote: > Newline between "class TargetMIPS32" and the start of the classes for insts. > > Also, we started to doxygenize some comments. Could you convert the "//" which > you think should appear in generated documentation into "///"? Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:56: // (TODO Reed Kotler) On 2015/07/07 18:02:35, jvoung wrote: > Please use a more consistent TODO marker: > > "TODO(reed kotler)" Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:64: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, On 2015/07/07 18:02:35, jvoung wrote: > LR -> RA in various places here and in the .cpp Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:262: UnimplementedError(Func->getContext()->getFlags()); On 2015/07/07 18:02:35, jvoung wrote: > Oh I see... previous comment about printing "jr $ra", you'll need to implement > this method to get that RA->emit(Func) to do the work. Is that possible within > this CL, or ? I'd rather do it in the next patch but I could do it now too if you would rather. I had some reason for skipping it in this patch but I can't remember why now. Maybe I was just in a hurry. :) https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; On 2015/07/07 18:02:35, jvoung wrote: > No need to do (void)Reg. You use it below in _ret(..., Reg); Done. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:35: ; MIPS32-NEXT: c: e7fedef0 On 2015/07/07 18:02:35, jvoung wrote: > I think the main TODO is that "e7fedef0" is an "undefined"/halting instruction > for ARM (what llvm.trap would translate to). Done. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:44: ; TOOD (Reed Kotler check this disassembly, .s file looks fine) On 2015/07/07 18:02:35, jvoung wrote: > Probably don't need a TODO, since this block doesn't have the iffy "e7fedef0" > check. Done.
Sorry for being so nitpicky about code style, but we try to be consistent. A few more comments regarding style below... Otherwise delaying the emitVariable seems fine to me. Thanks! https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py File pydir/run-pnacl-sz.py (right): https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#n... pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] } On 2015/07/07 21:54:54, reed.kotler wrote: > On 2015/07/07 18:02:34, jvoung wrote: > > Is it possible to leave out the "-none-nacl" for now, to be consistent with > the > > rest, or does that break the tests (if so, what does does the error look > like)? > > for now it's needed but I will try and eliminate it in the future. > > it will mess up the alignment in the assembler for nacl without it. Hmm okay, can't think of why the alignment would be different from the arm case, but could follow up on this later. You mean instruction bundling, etc.? I don't think the tests would be checking for that, except for a few tests like tests_lit/assembler/x86/sandboxing.ll, which we aren't running for ARM/MIPS. In those cases it would be looking for nops. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:44: << "jr $ra" On 2015/07/07 21:54:54, reed.kotler wrote: > On 2015/07/07 18:02:34, jvoung wrote: > > no need to print "$ra" here as part of "jr $ra" -- you have RA->emit(Func) > > below, which should print the $ra > > This is needed for now because emitVariable is not implemented yet. > I will remove it when I flesh out that function. Okay. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:28: // Base class for Mips instructions. On 2015/07/07 21:54:54, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > Newline between "class TargetMIPS32" and the start of the classes for insts. > > > > Also, we started to doxygenize some comments. Could you convert the "//" which > > you think should appear in generated documentation into "///"? > > Done. Doesn't appear to be inserted? https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:56: // (TODO Reed Kotler) On 2015/07/07 21:54:54, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > Please use a more consistent TODO marker: > > > > "TODO(reed kotler)" > > Done. "(TODO ...)" -> "TODO(...)" https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:64: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, On 2015/07/07 21:54:54, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > LR -> RA in various places here and in the .cpp > > Done. Doesn't appear to be done here (but done in other places)? https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:262: UnimplementedError(Func->getContext()->getFlags()); On 2015/07/07 21:54:55, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > Oh I see... previous comment about printing "jr $ra", you'll need to implement > > this method to get that RA->emit(Func) to do the work. Is that possible within > > this CL, or ? > > I'd rather do it in the next patch but I could do it now too if you would > rather. I had some reason for skipping it in this patch but I can't remember why > now. Maybe I was just in a hurry. :) Okay https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:359: ; On 2015/07/07 18:02:35, jvoung wrote: > you have an extra ";" here that can be removed Remember to remove this? https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:391: ; On 2015/07/07 18:02:35, jvoung wrote: > extra ";" Remember to remove this? https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; On 2015/07/07 21:54:55, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > No need to do (void)Reg. You use it below in _ret(..., Reg); > > Done. Doesn't appear to be removed? https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.h:79: void _ret(Variable *LR, Variable *Src0 = nullptr) { On 2015/07/07 18:02:35, jvoung wrote: > RA Could rename this too. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:44: ; TOOD (Reed Kotler check this disassembly, .s file looks fine) On 2015/07/07 21:54:55, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > Probably don't need a TODO, since this block doesn't have the iffy "e7fedef0" > > check. > > Done. Doesn't appear to be removed? https://codereview.chromium.org/1176133004/diff/60001/src/IceAssemblerMIPS32.h File src/IceAssemblerMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/60001/src/IceAssemblerMIPS32.... src/IceAssemblerMIPS32.h:55: // TBD (reed kotler) . Find out what this should be. "TBD (...)" -> "TODO(...)" https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:16: ; RUN: %if --need=target_MIPS32 --need=allow_dump --command %p2i --filetype=asm --assemble \ Since there is "\" at the end of the line to wrap and avoid spilling over 80 cols, you could wrap earlier. Otherwise this is now spilling over 80 cols. E.g., could wrap earlier like the ARM versions. https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:18: ; RUN: | %if --need=target_MIPS32 --need=allow_dump --command FileCheck --check-prefix MIPS32 %s similar, could wrap earlier https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:35: ; TODO (reed kotler check this disassembly, .s file looks fine) "TODO (reed kotler" -> "TODO(reed kotler)"
Sorry. There were two files that did not get saved in Eclipse for some reason. They were still modified in the editor. I use Eclipse and for some reason it greys out on our system when you save sometimes. I'm going though it all now. No need to apologize for nitpicking on code style. Those are easy to fix. I need to get used to your style on this project. n 2015/07/08 00:39:12, jvoung wrote: > Sorry for being so nitpicky about code style, but we try to be consistent. A few > more comments regarding style below... > > Otherwise delaying the emitVariable seems fine to me. > > Thanks! > > https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py > File pydir/run-pnacl-sz.py (right): > > https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#n... > pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] } > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:34, jvoung wrote: > > > Is it possible to leave out the "-none-nacl" for now, to be consistent with > > the > > > rest, or does that break the tests (if so, what does does the error look > > like)? > > > > for now it's needed but I will try and eliminate it in the future. > > > > it will mess up the alignment in the assembler for nacl without it. > > Hmm okay, can't think of why the alignment would be different from the arm case, > but could follow up on this later. You mean instruction bundling, etc.? I don't > think the tests would be checking for that, except for a few tests like > tests_lit/assembler/x86/sandboxing.ll, which we aren't running for ARM/MIPS. In > those cases it would be looking for nops. > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp > File src/IceInstMIPS32.cpp (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:44: << "jr $ra" > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:34, jvoung wrote: > > > no need to print "$ra" here as part of "jr $ra" -- you have RA->emit(Func) > > > below, which should print the $ra > > > > This is needed for now because emitVariable is not implemented yet. > > I will remove it when I flesh out that function. > > Okay. > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h > File src/IceInstMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:28: // Base class for Mips instructions. > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > Newline between "class TargetMIPS32" and the start of the classes for insts. > > > > > > Also, we started to doxygenize some comments. Could you convert the "//" > which > > > you think should appear in generated documentation into "///"? > > > > Done. > > Doesn't appear to be inserted? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:56: // (TODO Reed Kotler) > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > Please use a more consistent TODO marker: > > > > > > "TODO(reed kotler)" > > > > Done. > > "(TODO ...)" -> "TODO(...)" > > https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... > src/IceInstMIPS32.h:64: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > LR -> RA in various places here and in the .cpp > > > > Done. > > Doesn't appear to be done here (but done in other places)? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > File src/IceTargetLoweringMIPS32.cpp (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:262: > UnimplementedError(Func->getContext()->getFlags()); > On 2015/07/07 21:54:55, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > Oh I see... previous comment about printing "jr $ra", you'll need to > implement > > > this method to get that RA->emit(Func) to do the work. Is that possible > within > > > this CL, or ? > > > > I'd rather do it in the next patch but I could do it now too if you would > > rather. I had some reason for skipping it in this patch but I can't remember > why > > now. Maybe I was just in a hurry. :) > > Okay > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:359: ; > On 2015/07/07 18:02:35, jvoung wrote: > > you have an extra ";" here that can be removed > > Remember to remove this? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:391: ; > On 2015/07/07 18:02:35, jvoung wrote: > > extra ";" > > Remember to remove this? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; > On 2015/07/07 21:54:55, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > No need to do (void)Reg. You use it below in _ret(..., Reg); > > > > Done. > > Doesn't appear to be removed? > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > File src/IceTargetLoweringMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.h:79: void _ret(Variable *LR, Variable *Src0 = > nullptr) { > On 2015/07/07 18:02:35, jvoung wrote: > > RA > > Could rename this too. > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:44: ; TOOD (Reed Kotler check this > disassembly, .s file looks fine) > On 2015/07/07 21:54:55, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > Probably don't need a TODO, since this block doesn't have the iffy > "e7fedef0" > > > check. > > > > Done. > > Doesn't appear to be removed? > > https://codereview.chromium.org/1176133004/diff/60001/src/IceAssemblerMIPS32.h > File src/IceAssemblerMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/60001/src/IceAssemblerMIPS32.... > src/IceAssemblerMIPS32.h:55: // TBD (reed kotler) . Find out what this should > be. > "TBD (...)" -> "TODO(...)" > > https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:16: ; RUN: %if --need=target_MIPS32 > --need=allow_dump --command %p2i --filetype=asm --assemble \ > Since there is "\" at the end of the line to wrap and avoid spilling over 80 > cols, you could wrap earlier. Otherwise this is now spilling over 80 cols. > > E.g., could wrap earlier like the ARM versions. > > https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:18: ; RUN: | %if > --need=target_MIPS32 --need=allow_dump --command FileCheck --check-prefix MIPS32 > %s > similar, could wrap earlier > > https://codereview.chromium.org/1176133004/diff/60001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/function_aligned.ll:35: ; TODO (reed kotler check this > disassembly, .s file looks fine) > "TODO (reed kotler" -> "TODO(reed kotler)"
Hopefully I got all these formatting changes this time. https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py File pydir/run-pnacl-sz.py (right): https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#n... pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] } On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:34, jvoung wrote: > > > Is it possible to leave out the "-none-nacl" for now, to be consistent with > > the > > > rest, or does that break the tests (if so, what does does the error look > > like)? > > > > for now it's needed but I will try and eliminate it in the future. > > > > it will mess up the alignment in the assembler for nacl without it. > > Hmm okay, can't think of why the alignment would be different from the arm case, > but could follow up on this later. You mean instruction bundling, etc.? I don't > think the tests would be checking for that, except for a few tests like > tests_lit/assembler/x86/sandboxing.ll, which we aren't running for ARM/MIPS. In > those cases it would be looking for nops. Yes, I think it had to do with instruction bundling. I did this before vacation but if I remove it then a test fails now. I don't remember the issues right now. I need to go through a lot of things still; not just this. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:28: // Base class for Mips instructions. On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > Newline between "class TargetMIPS32" and the start of the classes for insts. > > > > > > Also, we started to doxygenize some comments. Could you convert the "//" > which > > > you think should appear in generated documentation into "///"? > > > > Done. > > Doesn't appear to be inserted? Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:56: // (TODO Reed Kotler) On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > Please use a more consistent TODO marker: > > > > > > "TODO(reed kotler)" > > > > Done. > > "(TODO ...)" -> "TODO(...)" Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:64: static InstMIPS32Ret *create(Cfg *Func, Variable *LR, On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 21:54:54, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > LR -> RA in various places here and in the .cpp > > > > Done. > > Doesn't appear to be done here (but done in other places)? Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:359: ; On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > you have an extra ";" here that can be removed > > Remember to remove this? Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:391: ; On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > extra ";" > > Remember to remove this? Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:618: (void)Reg; On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 21:54:55, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > No need to do (void)Reg. You use it below in _ret(..., Reg); > > > > Done. > > Doesn't appear to be removed? Done. https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.h:79: void _ret(Variable *LR, Variable *Src0 = nullptr) { On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > RA > > Could rename this too. Done. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:16: ; RUN: %if --need=target_MIPS32 --command %p2i --filetype=asm --assemble \ On 2015/07/07 18:02:35, jvoung wrote: > Can you add the "--need=allow_dump " that is now part of the ARM variant? I had > left that out previously. Done. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:18: ; RUN: | %if --need=target_MIPS32 --command FileCheck --check-prefix MIPS32 %s On 2015/07/07 18:02:35, jvoung wrote: > this line also requires a "--need=allow_dump" before the --command FileCheck Done. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:31: ; TODO (Reed Kotler check this disassembly, .s file looks fine) On 2015/07/07 18:02:35, jvoung wrote: > "TODO(reed kotler)" for consistency > > Or just leave out the TODO if it looks fine? Done. https://codereview.chromium.org/1176133004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/function_aligned.ll:44: ; TOOD (Reed Kotler check this disassembly, .s file looks fine) On 2015/07/08 00:39:11, jvoung wrote: > On 2015/07/07 21:54:55, reed.kotler wrote: > > On 2015/07/07 18:02:35, jvoung wrote: > > > Probably don't need a TODO, since this block doesn't have the iffy > "e7fedef0" > > > check. > > > > Done. > > Doesn't appear to be removed? Done. https://codereview.chromium.org/1176133004/diff/60001/src/IceAssemblerMIPS32.h File src/IceAssemblerMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/60001/src/IceAssemblerMIPS32.... src/IceAssemblerMIPS32.h:55: // TBD (reed kotler) . Find out what this should be. On 2015/07/08 00:39:12, jvoung wrote: > "TBD (...)" -> "TODO(...)" Done.
LGTM
https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes... File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes... tests_lit/llvm2ice_tests/function_aligned.ll:34: ; MIPS32: 4: {{.*}} jr ra About "Is it possible to leave out the "-none-nacl" for now", the reason it fails when you remove the "-none-nacl" is that this check is looking for "4: {{.*}} jr ra". If it was starting at "0:" then that would work with non-nacl. When the "check-lit" fails, it will print a whole slew of commands to reproduce the failure, and one of them is the mips invocation. Copy that and try it with/without the "-none-nacl". E.g., diff temp.nonnacl temp.nacl (git)-[reeds_change] 2c2 < /tmp/tmpRYvxTG: file format elf32-tradlittlemips-nacl --- > /tmp/tmp7nrsu2: file format elf32-tradlittlemips-nacl 8,10c8,10 < 0: 03e00008 jr ra < 4: 00000000 nop < 8: e7fedef0 swc1 $f30,-8464(ra) --- > 0: 03eef824 and ra,ra,t6 > 4: 03e00008 jr ra > 8: 00000000 nop 14,15c14,16 < 10: 03e00008 jr ra < 14: 00000000 nop --- > 10: 03eef824 and ra,ra,t6 > 14: 03e00008 jr ra > 18: 00000000 nop The nacl version has the masking for ra prior to the jump, which is why it is at 4: instead of at 0:. Unlike ARM and X86, the LLVM assembler for MIPS applies "auto-sandboxing". That is, besides inserting nops for alignment, it can also insert masking instructions (the ARM and X86 assemblers don't do that yet... since it can often involve a free register or reserving a register, it does require some cooperation with the compiler). Anyway, I'll help commit this as is, and you can adjust this in the next change, if you agree with the assessment.
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as d00d48da12ac44879066724978a032be64fcb0ad (presubmit successful).
Message was sent while issue was closed.
On 2015/07/08 16:44:26, jvoung wrote: > https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes... > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes... > tests_lit/llvm2ice_tests/function_aligned.ll:34: ; MIPS32: 4: {{.*}} jr ra > About "Is it possible to leave out the "-none-nacl" for now", the reason it > fails when you remove the "-none-nacl" is that this check is looking for "4: > {{.*}} jr ra". If it was starting at "0:" then that would work with non-nacl. > > When the "check-lit" fails, it will print a whole slew of commands to reproduce > the failure, and one of them is the mips invocation. Copy that and try it > with/without the "-none-nacl". E.g., > > diff temp.nonnacl temp.nacl > (git)-[reeds_change] > > 2c2 > < /tmp/tmpRYvxTG: file format elf32-tradlittlemips-nacl > --- > > /tmp/tmp7nrsu2: file format elf32-tradlittlemips-nacl > 8,10c8,10 > < 0: 03e00008 jr ra > < 4: 00000000 nop > < 8: e7fedef0 swc1 $f30,-8464(ra) > --- > > 0: 03eef824 and ra,ra,t6 > > 4: 03e00008 jr ra > > 8: 00000000 nop > 14,15c14,16 > < 10: 03e00008 jr ra > < 14: 00000000 nop > --- > > 10: 03eef824 and ra,ra,t6 > > 14: 03e00008 jr ra > > 18: 00000000 nop > > The nacl version has the masking for ra prior to the jump, which is why it is at > 4: instead of at 0:. Unlike ARM and X86, the LLVM assembler for MIPS applies > "auto-sandboxing". That is, besides inserting nops for alignment, it can also > insert masking instructions (the ARM and X86 assemblers don't do that yet... > since it can often involve a free register or reserving a register, it does > require some cooperation with the compiler). > > Anyway, I'll help commit this as is, and you can adjust this in the next change, > if you agree with the assessment. What is your proposal? I'm new to NACL/PNACL and am not conversant on the issues here. You want me to make that arch just be Mips and fix the offset in the test?
Message was sent while issue was closed.
I would like to resolve this target issue with -none-acl . Am requesting clarification on what is being requested of me here.
Message was sent while issue was closed.
On 2015/07/09 19:18:10, reed.kotler wrote: > On 2015/07/08 16:44:26, jvoung wrote: > > > https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes... > > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > > > > https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes... > > tests_lit/llvm2ice_tests/function_aligned.ll:34: ; MIPS32: 4: {{.*}} jr ra > > About "Is it possible to leave out the "-none-nacl" for now", the reason it > > fails when you remove the "-none-nacl" is that this check is looking for "4: > > {{.*}} jr ra". If it was starting at "0:" then that would work with non-nacl. > > > > When the "check-lit" fails, it will print a whole slew of commands to > reproduce > > the failure, and one of them is the mips invocation. Copy that and try it > > with/without the "-none-nacl". E.g., > > > > diff temp.nonnacl temp.nacl > > > > (git)-[reeds_change] > > > > 2c2 > > < /tmp/tmpRYvxTG: file format elf32-tradlittlemips-nacl > > --- > > > /tmp/tmp7nrsu2: file format elf32-tradlittlemips-nacl > > 8,10c8,10 > > < 0: 03e00008 jr ra > > < 4: 00000000 nop > > < 8: e7fedef0 swc1 $f30,-8464(ra) > > --- > > > 0: 03eef824 and ra,ra,t6 > > > 4: 03e00008 jr ra > > > 8: 00000000 nop > > 14,15c14,16 > > < 10: 03e00008 jr ra > > < 14: 00000000 nop > > --- > > > 10: 03eef824 and ra,ra,t6 > > > 14: 03e00008 jr ra > > > 18: 00000000 nop > > > > The nacl version has the masking for ra prior to the jump, which is why it is > at > > 4: instead of at 0:. Unlike ARM and X86, the LLVM assembler for MIPS applies > > "auto-sandboxing". That is, besides inserting nops for alignment, it can also > > insert masking instructions (the ARM and X86 assemblers don't do that yet... > > since it can often involve a free register or reserving a register, it does > > require some cooperation with the compiler). > > > > Anyway, I'll help commit this as is, and you can adjust this in the next > change, > > if you agree with the assessment. > > What is your proposal? > > I'm new to NACL/PNACL and am not conversant on the issues here. > > You want me to make that arch just be Mips and fix the offset in the test? Yes, my proposal is to remove the -none-nacl to be consistent with the rest, and fix the offsets in the test. pnacl-sz has a separate option to "--sandbox" and it should only do the equivalent of "-none-nacl" when that argument is given. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
