Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1640913004:
Subzero: Mips: Lower some i64 arithmetic instructions (Closed)
|
Created:
4 years, 10 months ago by rkotlerimgtec Modified:
4 years, 10 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. |
DescriptionSubzero: Mips: Lower some i64 arithmetic instructions.
This patch is a MIPS version of this part of ARM patch:
https://codereview.chromium.org/1151663004/
BUG=
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=00e360494341224e934a76446572864eef4f7db4
Patch Set 1 : First parallel patch to ARM Issue 1151663004 #
Total comments: 27
Patch Set 2 : changes suggested by stichnot #
Total comments: 26
Patch Set 3 : changes suggested by stichnot #
Total comments: 11
Patch Set 4 : changes suggested by stichnot and jpp #
Messages
Total messages: 23 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== temporary patch to get some help BUG= ========== to ========== Splitting up ARM patch into several sub patches. https://codereview.chromium.org/1151663004/ BUG= ==========
rkotlerimgtec@gmail.com changed reviewers: + jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org
stichnot@chromium.org changed reviewers: + eholk@chromium.org
Please edit the Description to make a suitable git commit message. E.g.: Subzero: Mips: Lower some i64 arithmetic instructions. Follows ARM issue xxx etc https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:589: Src0 = legalizeUndef(Src0); Can this be removed? It's already being done in the caller, and also it's strange to see this done for Src0 but not Src1. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:597: Variable *Src0LoR = legalizeToReg(Src0Lo); This can just be: Variable *Src0LoR = legalizeToReg(loOperand(Src0)); and Src0Lo above can be removed entirely. Same for the next 3 Variables. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:607: Variable *T_Lo = makeReg(IceType_i32); This lowering sequence has a problem in that invalid addressing modes might be generated if DestLo or DestHi are not register-allocated and therefore are mapped to stack locations. Something like this might be better: Variable *T_Carry = makeReg(IceType_i32); Variable *T_Lo = makeReg(IceType_i32); Variable *T_Hi = makeReg(IceType_i32); _addu(T_Lo, Src0LoR, Src1LoR); _mov(DestLo, T_Lo); _sltu(T_Carry, T_Lo, Src0LoR); _addu(T_Hi, T_Carry, Src0HiR); _addu(T_Hi, T_Hi, Src1HiR); _mov(DestHi, T_Hi); return; https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:616: _and(DestLo, Src0LoR, Src1LoR); Similar to above, you need to ensure the _and dest is a physical register. _and(T_Lo, Src0LoR, Src1LoR); _mov(DestLo, T_Lo); _and(T_Hi, Src0HiR, Src1HiR); _mov(DestHi, T_Hi); https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:621: Variable *T_Lo = makeReg(IceType_i32); Similar comment as for Add, maybe calling it "Borrow" instead of "Carry". https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:642: //} remove this https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:786: // TODO(jvoung): assign arguments to registers and stack. Also reserve stack. Don't add jvoung to a TODO. :) https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:791: // Generate the call instruction. Assign its result to a temporary reflow to 80-col https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:847: if (!Dest) Dest == nullptr https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1144: UnimplementedError(Func->getContext()->getFlags()); Remove this line, and replace it with "markRedefinitions();" https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1249: //_addiu(Reg, getPhysicalRegister(RegMIPS32::Reg_ZERO, Ty), Value); remove this https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right): https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:37: ; RUN: --command FileCheck --check-prefix MIPS32 --check-prefix MIPS32-O2 %s TIL that FileCheck can take multiple --check-prefix arguments. I wish I had known about this before! Three problems, though: 1. None of the check lines below use MIPS32-O2. 2. Many of the check lines below use MIPS which is not part of any RUN command. 3. You really need another RUN command that tests -Om1. https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:707: This and the next two tests add 2 blank lines instead of 1...
https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:589: Src0 = legalizeUndef(Src0); On 2016/01/30 17:03:28, stichnot wrote: > Can this be removed? It's already being done in the caller, and also it's > strange to see this done for Src0 but not Src1. Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:597: Variable *Src0LoR = legalizeToReg(Src0Lo); On 2016/01/30 17:03:28, stichnot wrote: > This can just be: > > Variable *Src0LoR = legalizeToReg(loOperand(Src0)); > > and Src0Lo above can be removed entirely. > > Same for the next 3 Variables. Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:607: Variable *T_Lo = makeReg(IceType_i32); On 2016/01/30 17:03:28, stichnot wrote: > This lowering sequence has a problem in that invalid addressing modes might be > generated if DestLo or DestHi are not register-allocated and therefore are > mapped to stack locations. > > Something like this might be better: > > Variable *T_Carry = makeReg(IceType_i32); > Variable *T_Lo = makeReg(IceType_i32); > Variable *T_Hi = makeReg(IceType_i32); > _addu(T_Lo, Src0LoR, Src1LoR); > _mov(DestLo, T_Lo); > _sltu(T_Carry, T_Lo, Src0LoR); > _addu(T_Hi, T_Carry, Src0HiR); > _addu(T_Hi, T_Hi, Src1HiR); > _mov(DestHi, T_Hi); > return; Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:616: _and(DestLo, Src0LoR, Src1LoR); On 2016/01/30 17:03:28, stichnot wrote: > Similar to above, you need to ensure the _and dest is a physical register. > > _and(T_Lo, Src0LoR, Src1LoR); > _mov(DestLo, T_Lo); > _and(T_Hi, Src0HiR, Src1HiR); > _mov(DestHi, T_Hi); Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:616: _and(DestLo, Src0LoR, Src1LoR); On 2016/01/30 17:03:28, stichnot wrote: > Similar to above, you need to ensure the _and dest is a physical register. > > _and(T_Lo, Src0LoR, Src1LoR); > _mov(DestLo, T_Lo); > _and(T_Hi, Src0HiR, Src1HiR); > _mov(DestHi, T_Hi); Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:621: Variable *T_Lo = makeReg(IceType_i32); On 2016/01/30 17:03:29, stichnot wrote: > Similar comment as for Add, maybe calling it "Borrow" instead of "Carry". Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:642: //} On 2016/01/30 17:03:29, stichnot wrote: > remove this Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:786: // TODO(jvoung): assign arguments to registers and stack. Also reserve stack. On 2016/01/30 17:03:28, stichnot wrote: > Don't add jvoung to a TODO. :) Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:791: // Generate the call instruction. Assign its result to a temporary On 2016/01/30 17:03:28, stichnot wrote: > reflow to 80-col Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:847: if (!Dest) On 2016/01/30 17:03:28, stichnot wrote: > Dest == nullptr Done. https://codereview.chromium.org/1640913004/diff/60001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1249: //_addiu(Reg, getPhysicalRegister(RegMIPS32::Reg_ZERO, Ty), Value); On 2016/01/30 17:03:28, stichnot wrote: > remove this Done. https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right): https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:37: ; RUN: --command FileCheck --check-prefix MIPS32 --check-prefix MIPS32-O2 %s On 2016/01/30 17:03:29, stichnot wrote: > TIL that FileCheck can take multiple --check-prefix arguments. I wish I had > known about this before! > > Three problems, though: > > 1. None of the check lines below use MIPS32-O2. > > 2. Many of the check lines below use MIPS which is not part of any RUN command. > > 3. You really need another RUN command that tests -Om1. This just mimicks what ARM is doing here. -Om1 was not in the ARM patch that this follows. There are some things regarding the stack that need to be there that had not been implemented at this stage of ARM development. https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:707: On 2016/01/30 17:03:29, stichnot wrote: > This and the next two tests add 2 blank lines instead of 1... Done.
https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right): https://codereview.chromium.org/1640913004/diff/60001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:37: ; RUN: --command FileCheck --check-prefix MIPS32 --check-prefix MIPS32-O2 %s On 2016/01/30 23:48:31, rkotlerimgtec wrote: > On 2016/01/30 17:03:29, stichnot wrote: > > TIL that FileCheck can take multiple --check-prefix arguments. I wish I had > > known about this before! > > > > Three problems, though: > > > > 1. None of the check lines below use MIPS32-O2. > > > > 2. Many of the check lines below use MIPS which is not part of any RUN > command. > > > > 3. You really need another RUN command that tests -Om1. > > This just mimicks what ARM is doing here. -Om1 was not in the ARM > patch that this follows. There are some things regarding the stack that need to > be there that had not been implemented at this stage of ARM development. All the same, using -Om1 exposes lowering/legalization problems (operands that should be in registers but aren't) and this is an ideal setting to find and fix them. Spoiler alert: if you change it to -Om1 here and make all the other changes I suggested, you will get a passing test. :) https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:591: Variable *Src0LoR = legalizeToReg(loOperand(Src0), Legal_Reg); Remove the Legal_Reg optional argument. That argument slot is supposed to be for a pre-colored register number. Too bad the compiler doesn't complain about a value from the wrong enum type. https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:604: Variable *T_Hi2 = makeReg(IceType_i32); You can remove this T_Hi2 and reuse T_Hi instead. The x86 lowering does this a lot. But you will need to apply the change I pointed out in postLower(), or you get some kind of liveness error. https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:607: _sltu(T_Carry, DestLo, Src0LoR); Use T_Lo instead of DestLo here, since DestLo isn't guaranteed to be a physical register but T_Lo is. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right): https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:34: ; RUN: --disassemble --target mips32 -i %s --args -O2 --skip-unimplemented \ Nit: there are two spaces between "--args" and "-O2". https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:50: ; MIPS: move v0,a2 There is no "--check-prefix MIPS" so this check line is always ignored. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:186: ; MIPS: jr ra There is no "--check-prefix MIPS" so this check line is always ignored. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:205: ; MIPS-LABEL; return64BitArg There is no "--check-prefix MIPS" so this check line is always ignored. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:252: ; MIPS-LABEL: add64BitSigned There is no "--check-prefix MIPS" so this check line is always ignored. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:275: ; MIPS-LABEL: add64BitUnsigned There is no "--check-prefix MIPS" so this check line is always ignored. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:298: ; MIPS-LABEL: sub64BitSigned There is no "--check-prefix MIPS" so this check line is always ignored. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:321: ; MIPS-LABEL: sub64BitUnsigned There is no "--check-prefix MIPS" so this check line is always ignored. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:727: Two blank lines should be just one. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:749: Two blank lines should be just one.
I would prefer to deal with -Om1 issues in a later patch. This has several issues that have to do with the stack and that code does not appear in the ARM patches until later either. https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:591: Variable *Src0LoR = legalizeToReg(loOperand(Src0), Legal_Reg); On 2016/01/31 05:29:47, stichnot wrote: > Remove the Legal_Reg optional argument. > > That argument slot is supposed to be for a pre-colored register number. Too bad > the compiler doesn't complain about a value from the wrong enum type. Done. https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:604: Variable *T_Hi2 = makeReg(IceType_i32); On 2016/01/31 05:29:47, stichnot wrote: > You can remove this T_Hi2 and reuse T_Hi instead. The x86 lowering does this a > lot. But you will need to apply the change I pointed out in postLower(), or you > get some kind of liveness error. I would rather address this issue in another patch. I see how the this is handled in the other ports but would like to better understand the details before adding it to the MIPS one. https://codereview.chromium.org/1640913004/diff/80001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:607: _sltu(T_Carry, DestLo, Src0LoR); On 2016/01/31 05:29:47, stichnot wrote: > Use T_Lo instead of DestLo here, since DestLo isn't guaranteed to be a physical > register but T_Lo is. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/64bit.pnacl.ll (right): https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:34: ; RUN: --disassemble --target mips32 -i %s --args -O2 --skip-unimplemented \ On 2016/01/31 05:29:47, stichnot wrote: > Nit: there are two spaces between "--args" and "-O2". Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:50: ; MIPS: move v0,a2 On 2016/01/31 05:29:48, stichnot wrote: > There is no "--check-prefix MIPS" so this check line is always ignored. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:186: ; MIPS: jr ra On 2016/01/31 05:29:47, stichnot wrote: > There is no "--check-prefix MIPS" so this check line is always ignored. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:205: ; MIPS-LABEL; return64BitArg On 2016/01/31 05:29:47, stichnot wrote: > There is no "--check-prefix MIPS" so this check line is always ignored. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:252: ; MIPS-LABEL: add64BitSigned On 2016/01/31 05:29:47, stichnot wrote: > There is no "--check-prefix MIPS" so this check line is always ignored. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:275: ; MIPS-LABEL: add64BitUnsigned On 2016/01/31 05:29:47, stichnot wrote: > There is no "--check-prefix MIPS" so this check line is always ignored. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:298: ; MIPS-LABEL: sub64BitSigned On 2016/01/31 05:29:47, stichnot wrote: > There is no "--check-prefix MIPS" so this check line is always ignored. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:321: ; MIPS-LABEL: sub64BitUnsigned On 2016/01/31 05:29:47, stichnot wrote: > There is no "--check-prefix MIPS" so this check line is always ignored. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:727: On 2016/01/31 05:29:47, stichnot wrote: > Two blank lines should be just one. Done. https://codereview.chromium.org/1640913004/diff/80001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/64bit.pnacl.ll:749: On 2016/01/31 05:29:47, stichnot wrote: > Two blank lines should be just one. Done.
On 2016/01/31 21:06:13, rkotlerimgtec wrote: > I would prefer to deal with -Om1 issues in a later patch. This has several > issues that have to do with the stack and that code does not appear in the ARM > patches until later either. I guess this is OK, but I think you are missing a good opportunity to find and fix these legalization problems when the lowering code is still fresh. I'm curious what the stack issues are (so that I can pay attention in later CLs). I know there is the issue of far offsets from the stack/frame pointer (which generally doesn't show up in tests unless you manually force the issue). Are there any others? Otherwise, using -Om1 passes the tests except for a couple that assume better register allocation than Om1 provides.
Because of stack/frame issues, it's going to generate some code that will not assemble. The make check now actually assembles and disassembles code so this is a problem. I actually was going to make the patch for that ages ago but had decided to track the ARM patches. I'm tracking the ARM port patches so some things were not working in the ARM port at this time and so they don't work in the Mips port yet either. On Sun, Jan 31, 2016 at 7:47 PM, <stichnot@chromium.org> wrote: > On 2016/01/31 21:06:13, rkotlerimgtec wrote: > > I would prefer to deal with -Om1 issues in a later patch. This has several > > issues that have to do with the stack and that code does not appear in the ARM > > patches until later either. > > I guess this is OK, but I think you are missing a good opportunity to find and > fix these legalization problems when the lowering code is still fresh. > > I'm curious what the stack issues are (so that I can pay attention in later > CLs). I know there is the issue of far offsets from the stack/frame pointer > (which generally doesn't show up in tests unless you manually force the issue). > Are there any others? > > Otherwise, using -Om1 passes the tests except for a couple that assume better > register allocation than Om1 provides. > https://codereview.chromium.org/1640913004/ > > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
https://codereview.chromium.org/1640913004/diff/100001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/100001/src/IceInstMIPS32.cpp#... src/IceInstMIPS32.cpp:165: llvm::report_fatal_error("ARM32Call to ConstantInteger32"); MIPS32Call https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:704: _add(T, Src0R, Src1R); Should this be addu instead? Same for sub https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:857: Inst *FakeUse = InstFakeUse::create(Func, ReturnReg); You could have done Context.insert<InstFakeDef>(Func, ReturnReg) https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:884: UnimplementedLoweringError(this, Instr); Do you really want to report an error here? https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:1156: // and set the IsDestRedefined flag to keep liveness analysis consistent. You're better off explicitly setting the bit during lowering, not after the fact. Setting the bit here night mask some lowering mistakes that you overlooked.
It will get fixed in a few patches from now. There is all ARM -O2 up to this point and that is for a reason; the same reason. On Sun, Jan 31, 2016 at 8:25 PM, reed kotler <rkotlerimgtec@gmail.com> wrote: > Because of stack/frame issues, it's going to generate some code that will > not assemble. The make check now actually assembles and disassembles code > so this is a problem. > I actually was going to make the patch for that ages ago but had decided > to track the ARM patches. > > I'm tracking the ARM port patches so some things were not working in the > ARM port at this time and so they don't work in the Mips port yet either. > > On Sun, Jan 31, 2016 at 7:47 PM, <stichnot@chromium.org> wrote: > >> On 2016/01/31 21:06:13, rkotlerimgtec wrote: >> > I would prefer to deal with -Om1 issues in a later patch. This has several >> > issues that have to do with the stack and that code does not appear in the ARM >> > patches until later either. >> >> I guess this is OK, but I think you are missing a good opportunity to find and >> fix these legalization problems when the lowering code is still fresh. >> >> I'm curious what the stack issues are (so that I can pay attention in later >> CLs). I know there is the issue of far offsets from the stack/frame pointer >> (which generally doesn't show up in tests unless you manually force the issue). >> Are there any others? >> >> Otherwise, using -Om1 passes the tests except for a couple that assume better >> register allocation than Om1 provides. >> https://codereview.chromium.org/1640913004/ >> >> > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at https://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/d/optout.
https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:1156: // and set the IsDestRedefined flag to keep liveness analysis consistent. On 2016/02/01 04:29:32, John wrote: > You're better off explicitly setting the bit during lowering, not after the > fact. Setting the bit here night mask some lowering mistakes that you > overlooked. What this TODO really means, I think, is to call TargetLowering::markRedefinitions() here just like what's done in the ARM and x86 targets. This would be in contrast to the situations where the redefinition has to be explicitly marked such as predicated moves or intra-block control flow.
https://codereview.chromium.org/1640913004/diff/100001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/100001/src/IceInstMIPS32.cpp#... src/IceInstMIPS32.cpp:165: llvm::report_fatal_error("ARM32Call to ConstantInteger32"); On 2016/02/01 04:29:32, John wrote: > MIPS32Call Done. https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:704: _add(T, Src0R, Src1R); On 2016/02/01 04:29:32, John wrote: > Should this be addu instead? Same for sub Done. https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:857: Inst *FakeUse = InstFakeUse::create(Func, ReturnReg); On 2016/02/01 04:29:32, John wrote: > You could have done > > Context.insert<InstFakeDef>(Func, ReturnReg) Done. https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:884: UnimplementedLoweringError(this, Instr); On 2016/02/01 04:29:32, John wrote: > Do you really want to report an error here? Done. https://codereview.chromium.org/1640913004/diff/100001/src/IceTargetLoweringM... src/IceTargetLoweringMIPS32.cpp:1156: // and set the IsDestRedefined flag to keep liveness analysis consistent. On 2016/02/01 04:57:50, stichnot wrote: > On 2016/02/01 04:29:32, John wrote: > > You're better off explicitly setting the bit during lowering, not after the > > fact. Setting the bit here night mask some lowering mistakes that you > > overlooked. > > What this TODO really means, I think, is to call > TargetLowering::markRedefinitions() here just like what's done in the ARM and > x86 targets. > > This would be in contrast to the situations where the redefinition has to be > explicitly marked such as predicated moves or intra-block control flow. Yes. The intention is just to do what ARM does here but I want to do it in a follow on patch after studying this a bit.
LGTM. However, I want to restate that it's important to set a clear, <80-column, one-line git summary of the change, which is the first line of the Description field. (Followed by a blank line and then a more detailed description.) E.g., if you do "git log --oneline", you should see a reasonable high-level one-line description of the commit. In this case, "Splitting up ARM patch into several sub patches." is not going to mean much to most people. Something better might be "Subzero: Mips: Lower some i64 arithmetic instructions." (I tend to use the "Subzero" prefix because the checkin emails go to the same address as all other Native Client checkin emails.)
Description was changed from ========== Splitting up ARM patch into several sub patches. https://codereview.chromium.org/1151663004/ BUG= ========== to ========== This patch is a MIPS version of this part of ARM patch: https://codereview.chromium.org/1151663004/ BUG= ==========
Description was changed from ========== This patch is a MIPS version of this part of ARM patch: https://codereview.chromium.org/1151663004/ BUG= ========== to ========== Subzero: Mips: Lower some i64 arithmetic instructions. This patch is a MIPS version of this part of ARM patch: https://codereview.chromium.org/1151663004/ BUG= ==========
Description was changed from ========== Subzero: Mips: Lower some i64 arithmetic instructions. This patch is a MIPS version of this part of ARM patch: https://codereview.chromium.org/1151663004/ BUG= ========== to ========== Subzero: Mips: Lower some i64 arithmetic instructions. This patch is a MIPS version of this part of ARM patch: https://codereview.chromium.org/1151663004/ BUG= R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) manually as 00e360494341224e934a76446572864eef4f7db4 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
