Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1898743002:
[Subzero][MIPS] Implement conditional branches and integer comparisons (Closed)
|
Created:
4 years, 8 months ago by sagar.thakur Modified:
4 years, 8 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com, srdjan.obucina_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
Description[Subzero][MIPS] Implement conditional branches and integer comparisons
R=stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=1a478b1ce5538d4da48ca65b321218d584fd360a
Patch Set 1 #
Total comments: 21
Patch Set 2 : Addressed review comments #
Messages
Total messages: 13 (4 generated)
sagar.thakur@imgtec.com changed reviewers: + stichnot@chromium.org
Description was changed from ========== [Subzero][MIPS] Implement conditional branches and interger comparitions ========== to ========== [Subzero][MIPS] Implement conditional branches and integer comparisons ==========
stichnot@chromium.org changed reviewers: + eholk@chromium.org, jpp@chromium.org, kschimpf@google.com
Just saw this in my queue, thanks! Be sure to "Publish+Mail Comments" when the CL is ready for review, which is the usual way of notifying reviewers of a new CL, or that a new patchset has been uploaded for review. Also, I updated the reviewer list; it's best to include all 4 on Subzero CLs.
https://codereview.chromium.org/1898743002/diff/1/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1898743002/diff/1/src/IceInstMIPS32.h#newcode142 src/IceInstMIPS32.h:142: Slt, Alphabetize this list. (also fixing the incorrect sorting of "Sltu"...) https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:964: void TargetMIPS32::lowerIcmp(const InstIcmp *Instr) { This CL is currently lacking tests. There should be a basic lit test for each icmp condition. Unfortunately, the tests_lit/llvm2ice_tests/ directory doesn't seem to have a suitable test file to add MIPS tests to, so I think you need to create a new one. You might start with vector-icmp.ll as a template. "make -f Makefile.standalone check-lit" to run the tests. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:967: if (Src0->getType() == IceType_i64 || Src1->getType() == IceType_i64) { Typically we just check the type of getDest() or getSrc(0) when dispatching on type - and of course it can't be getDest() in this case. It would be fine to assert in lower64Icmp() that Src1 has type i64. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:968: lower64Icmp(Instr); LLVM coding style uses 2-space indents. If you run "make -f Makefile.standalone format", it will take care of that for you. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:977: auto Src0R = legalizeToReg(Src0); auto * for these two https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:981: auto DestT = I32Reg(), T = I32Reg(); We usually do variable initializations as separate statements: auto *DestT = I32Reg(); auto *T = I32Reg(); Also, all of these "auto"s should be "auto *". https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:988: auto DestT = I32Reg(), T = I32Reg(), Zero = getZero(); The following doesn't need to be addressed in this CL, but it's time to start thinking about it. How do you plan to deal with comparisons and other arithmetic on i8 and i16 operands? (and perhaps i1 as well?) For x86, we use native 8-bit and 16-bit operations on i8 and i16 operands. We have to deal with the complexities of extra restrictions on some i8 operations, which makes the lowering code a bit more complex. We also have to deal with register classes and register aliasing, but that is handled pretty cleanly for x86 and ARM, and will presumably work just as well for MIPS. Another approach is to make an invariant that all i8 and i16 source operands are already sign-extended or zero-extended to i32. Naively done, this might result in a lot of redundant sign- or zero-extensions that would need separate analysis to clean up. Yet another approach might be to assume that any i8 or i16 source operand resides in a 32-bit slot (stack location or register), and simply has garbage in the upper bits. Some operations, like add, may not care about the upper bits, which other operations, like icmp, may need to first sign- or zero-extend the source operands first. (This may actually be a very reasonable approach, and I'm sorry I didn't consider it for the x86 lowering...) https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:1050: (void)Src0; remove these https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:1053: UnimplementedLoweringError(this, Instr); I think this can be removed, given the return in the default clause. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32.h File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.h:219: void _sub(Variable *Dest, Variable *Src0, Variable *Src1) { Move this to be just before _sltu for sorting consistency.
https://codereview.chromium.org/1898743002/diff/1/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1898743002/diff/1/src/IceInstMIPS32.h#newcode142 src/IceInstMIPS32.h:142: Slt, On 2016/04/19 16:44:01, stichnot wrote: > Alphabetize this list. (also fixing the incorrect sorting of "Sltu"...) Done. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:964: void TargetMIPS32::lowerIcmp(const InstIcmp *Instr) { On 2016/04/19 16:44:01, stichnot wrote: > This CL is currently lacking tests. There should be a basic lit test for each > icmp condition. > > Unfortunately, the tests_lit/llvm2ice_tests/ directory doesn't seem to have a > suitable test file to add MIPS tests to, so I think you need to create a new > one. You might start with vector-icmp.ll as a template. > > "make -f Makefile.standalone check-lit" to run the tests. Yes, the test for icmp eq could be something like this : define internal i32 @test_icmp_eq(i32 %a, i32 %b) { entry: %icmp = icmp eq i32 %a, %b %res = zext i1 %icmp to i32 ret i32 %res } But, with this test subzero throws an unimplemented error message from the lowering code for casts. We also get unimplemented message from TargetMIPS32::emitVariable(). Therefore, I think we should first implement these missing bits before we can add these tests. What do you think ? https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:967: if (Src0->getType() == IceType_i64 || Src1->getType() == IceType_i64) { On 2016/04/19 16:44:01, stichnot wrote: > Typically we just check the type of getDest() or getSrc(0) when dispatching on > type - and of course it can't be getDest() in this case. It would be fine to > assert in lower64Icmp() that Src1 has type i64. Done. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:968: lower64Icmp(Instr); On 2016/04/19 16:44:02, stichnot wrote: > LLVM coding style uses 2-space indents. If you run "make -f Makefile.standalone > format", it will take care of that for you. Done. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:977: auto Src0R = legalizeToReg(Src0); On 2016/04/19 16:44:02, stichnot wrote: > auto * > for these two Done. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:981: auto DestT = I32Reg(), T = I32Reg(); On 2016/04/19 16:44:01, stichnot wrote: > We usually do variable initializations as separate statements: > auto *DestT = I32Reg(); > auto *T = I32Reg(); > > Also, all of these "auto"s should be "auto *". Done. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:988: auto DestT = I32Reg(), T = I32Reg(), Zero = getZero(); On 2016/04/19 16:44:01, stichnot wrote: > The following doesn't need to be addressed in this CL, but it's time to start > thinking about it. > > How do you plan to deal with comparisons and other arithmetic on i8 and i16 > operands? (and perhaps i1 as well?) > > For x86, we use native 8-bit and 16-bit operations on i8 and i16 operands. We > have to deal with the complexities of extra restrictions on some i8 operations, > which makes the lowering code a bit more complex. We also have to deal with > register classes and register aliasing, but that is handled pretty cleanly for > x86 and ARM, and will presumably work just as well for MIPS. > > Another approach is to make an invariant that all i8 and i16 source operands are > already sign-extended or zero-extended to i32. Naively done, this might result > in a lot of redundant sign- or zero-extensions that would need separate analysis > to clean up. > > Yet another approach might be to assume that any i8 or i16 source operand > resides in a 32-bit slot (stack location or register), and simply has garbage in > the upper bits. Some operations, like add, may not care about the upper bits, > which other operations, like icmp, may need to first sign- or zero-extend the > source operands first. (This may actually be a very reasonable approach, and > I'm sorry I didn't consider it for the x86 lowering...) I think the third approach would be a good one. With this approach we wouldn't need to worry about redundant sign- or zero-extensions. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:1050: (void)Src0; On 2016/04/19 16:44:02, stichnot wrote: > remove these Done. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:1053: UnimplementedLoweringError(this, Instr); On 2016/04/19 16:44:01, stichnot wrote: > I think this can be removed, given the return in the default clause. Done. https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32.h File src/IceTargetLoweringMIPS32.h (right): https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.h:219: void _sub(Variable *Dest, Variable *Src0, Variable *Src1) { On 2016/04/19 16:44:02, stichnot wrote: > Move this to be just before _sltu for sorting consistency. Done.
lgtm https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1898743002/diff/1/src/IceTargetLoweringMIPS32... src/IceTargetLoweringMIPS32.cpp:964: void TargetMIPS32::lowerIcmp(const InstIcmp *Instr) { On 2016/04/25 09:14:32, sagar.thakur wrote: > On 2016/04/19 16:44:01, stichnot wrote: > > This CL is currently lacking tests. There should be a basic lit test for each > > icmp condition. > > > > Unfortunately, the tests_lit/llvm2ice_tests/ directory doesn't seem to have a > > suitable test file to add MIPS tests to, so I think you need to create a new > > one. You might start with vector-icmp.ll as a template. > > > > "make -f Makefile.standalone check-lit" to run the tests. > > Yes, the test for icmp eq could be something like this : > > define internal i32 @test_icmp_eq(i32 %a, i32 %b) { > entry: > %icmp = icmp eq i32 %a, %b > %res = zext i1 %icmp to i32 > ret i32 %res > } > > But, with this test subzero throws an unimplemented error message from the > lowering code for casts. We also get unimplemented message from > TargetMIPS32::emitVariable(). Therefore, I think we should first implement these > missing bits before we can add these tests. What do you think ? Ah, you're right. It looks like none of the lowering is implemented yet that would consume an i1 operand. So you could have a test that does an icmp and doesn't use the result, but then liveness analysis with -O2 would remove the icmp instruction. So I guess we can wait for conditional branches, sext, or zext to add icmp tests.
Description was changed from ========== [Subzero][MIPS] Implement conditional branches and integer comparisons ========== to ========== [Subzero][MIPS] Implement conditional branches and integer comparisons 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 #2 (id:20001) manually as 1a478b1ce5538d4da48ca65b321218d584fd360a (presubmit successful).
Message was sent while issue was closed.
Hi Jim, Is it possible to add a comment that this patch was based on work that Reed Kotler did? -rich -------- Original message -------- From: "stichnot@chromium.org via codereview.chromium.org" <reply@chromiumcodereview-hr.appspotmail.com> Date: 4/25/16 08:39 (GMT-08:00) To: Sagar Thakur <Sagar.Thakur@imgtec.com>, jpp@chromium.org, eholk@chromium.org, kschimpf@google.com, stichnot@chromium.org Cc: native-client-reviews@googlegroups.com, Rich Fuhler <Rich.Fuhler@imgtec.com>, Srdjan Obucina <Srdjan.Obucina@imgtec.com> Subject: Re: [Subzero][MIPS] Implement conditional branches and integer comparisons (issue 1898743002 by sagar.thakur@imgtec.com) Committed patchset #2 (id:20001) manually as 1a478b1ce5538d4da48ca65b321218d584fd360a (presubmit successful). https://codereview.chromium.org/1898743002/ -- 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.
Message was sent while issue was closed.
Hi Rich, We can't really rewrite git history on the central repo (or at least, we have no practice doing so and it's hard to predict the problems that might cause). The best suggestion I've heard is to create a new CL to revert this one, followed by another CL that re-implements it with the corrected description. That way, it shows up prominently in "git log", and is likely to show up in "git blame". Let me know if you want this, and I will take care of the revert CL. On Tue, Apr 26, 2016 at 12:11 PM, Rich Fuhler <Rich.Fuhler@imgtec.com> wrote: > Hi Jim, > > Is it possible to add a comment that this patch was based on work that > Reed Kotler did? > > > -rich > > > -------- Original message -------- > From: "stichnot@chromium.org via codereview.chromium.org" < > reply@chromiumcodereview-hr.appspotmail.com> > Date: 4/25/16 08:39 (GMT-08:00) > To: Sagar Thakur <Sagar.Thakur@imgtec.com>, jpp@chromium.org, > eholk@chromium.org, kschimpf@google.com, stichnot@chromium.org > Cc: native-client-reviews@googlegroups.com, Rich Fuhler < > Rich.Fuhler@imgtec.com>, Srdjan Obucina <Srdjan.Obucina@imgtec.com> > Subject: Re: [Subzero][MIPS] Implement conditional branches and integer > comparisons (issue 1898743002 by sagar.thakur@imgtec.com) > > Committed patchset #2 (id:20001) manually as > 1a478b1ce5538d4da48ca65b321218d584fd360a (presubmit successful). > > https://codereview.chromium.org/1898743002/ > -- 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.
Message was sent while issue was closed.
Hi Jim, Leave it as is - no need to revert it. -rich -------- Original message -------- From: Jim Stichnoth <stichnot@google.com> Date: 4/26/16 14:09 (GMT-08:00) To: Rich Fuhler <Rich.Fuhler@imgtec.com> Cc: Sagar Thakur <Sagar.Thakur@imgtec.com>, jpp@chromium.org, eholk@chromium.org, kschimpf@google.com, native-client-reviews@googlegroups.com, Srdjan Obucina <Srdjan.Obucina@imgtec.com> Subject: Re: [Subzero][MIPS] Implement conditional branches and integer comparisons (issue 1898743002 by sagar.thakur@imgtec.com) Hi Rich, We can't really rewrite git history on the central repo (or at least, we have no practice doing so and it's hard to predict the problems that might cause). The best suggestion I've heard is to create a new CL to revert this one, followed by another CL that re-implements it with the corrected description. That way, it shows up prominently in "git log", and is likely to show up in "git blame". Let me know if you want this, and I will take care of the revert CL. On Tue, Apr 26, 2016 at 12:11 PM, Rich Fuhler <Rich.Fuhler@imgtec.com<mailto:Rich.Fuhler@imgtec.com>> wrote: Hi Jim, Is it possible to add a comment that this patch was based on work that Reed Kotler did? -rich -------- Original message -------- From: "stichnot@chromium.org<mailto:stichnot@chromium.org> via codereview.chromium.org<http://codereview.chromium.org>" <reply@chromiumcodereview-hr.appspotmail.com<mailto:reply@chromiumcodereview-hr.appspotmail.com>> Date: 4/25/16 08:39 (GMT-08:00) To: Sagar Thakur <Sagar.Thakur@imgtec.com<mailto:Sagar.Thakur@imgtec.com>>, jpp@chromium.org<mailto:jpp@chromium.org>, eholk@chromium.org<mailto:eholk@chromium.org>, kschimpf@google.com<mailto:kschimpf@google.com>, stichnot@chromium.org<mailto:stichnot@chromium.org> Cc: native-client-reviews@googlegroups.com<mailto:native-client-reviews@googlegroups.com>, Rich Fuhler <Rich.Fuhler@imgtec.com<mailto:Rich.Fuhler@imgtec.com>>, Srdjan Obucina <Srdjan.Obucina@imgtec.com<mailto:Srdjan.Obucina@imgtec.com>> Subject: Re: [Subzero][MIPS] Implement conditional branches and integer comparisons (issue 1898743002 by sagar.thakur@imgtec.com<mailto:sagar.thakur@imgtec.com>) Committed patchset #2 (id:20001) manually as 1a478b1ce5538d4da48ca65b321218d584fd360a (presubmit successful). https://codereview.chromium.org/1898743002/ -- 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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
