Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 7 months ago by sagar.thakur Modified:
4 years, 7 months ago Reviewers:
Eric Holk, Karl, Jim Stichnoth, John CC:
native-client-reviews_googlegroups.com, srdjan.obucina_imgtec.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdded bool folding machinery for MIPS32.
Added implementation for conditional branch instructions.
R=jpp@chromium.org, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=5cce76190a9bd6a6a524b2afee917f6bec922d8d
Patch Set 1 #Patch Set 2 : Added tests for eq and ne branches and corrected branch target label emission #
Total comments: 31
Patch Set 3 : Addressed review comments #
Total comments: 10
Patch Set 4 : Addressed review comments #
Total comments: 2
Created: 4 years, 7 months ago
Messages
Total messages: 14 (3 generated)
Description was changed from ========== [Subzero][MIPS32] Addition of bool folding machinery and implementation of conditional branches BUG= ========== to ========== Added bool folding machinery for MIPS32. Added implementation for conditional branch instructions. ==========
sagar.thakur@imgtec.com changed reviewers: + eholk@chromium.org, jpp@chromium.org, kschimpf@google.com, stichnot@chromium.org
lgtm https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:888: return; no need to "return" here. https://codereview.chromium.org/1993773004/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/cond-branch.ll (right): https://codereview.chromium.org/1993773004/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/cond-branch.ll:3: ; RUN: %if --need=allow_dump --need=target_MIPS32 --command %p2i --filetype=asm \ 80-col
https://codereview.chromium.org/1993773004/diff/20001/src/IceConditionCodesMI... File src/IceConditionCodesMIPS32.h (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceConditionCodesMI... src/IceConditionCodesMIPS32.h:1: //===- subzero/src/IceConditionCodesMIPS32.h - Condition Codes ---*- C++ Fix clang-format's line wrapping - probably change '---' to '--' and put everything back onto a single line. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:126: Operand *Src1, const InstMIPS32Label *Label, In ICE there's a pretty strong assumption/pattern that none of the instruction src operands are nullptr. I think it would be cleaner (specifically for the callers of this ctor) if you did one of two things: (1) Move Src0 and Src1 to the end of the arg list and make Src1 be an optional parameter with a default of nullptr. (2) Have separate ctors for the 1 and 2 src arg cases. Personally I would prefer this option. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:131: if (Src1 != NULL) nullptr https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:328: return; this "return" is unnecessary, remove https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:1: //===- subzero/src/IceInstMIPS32.def - X-Macros for MIPS32 insts --*- C++ -*-===// Fix this line to be 80-col - maybe "MIPS32 insts" should become "MIPS insts"? Need to keep the magic "-* -C++ -*-" sequence for emacs, though it looks like the space characters in that sequence might be optional. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:175: /* enum value, opposite, emit */ \ The table has 4 columns, but this comment only documents 3 of them? Assuming this is modeled after the ARM table, the second column is supposed to be the value used in the instruction encoding. If it's easier to just delete the encoding column for now, that's fine. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:176: X(EQ, 0, NE, "eq") /* equal */ \ Optional: add extra spaces so the columns line up nicely. (Note that .def files are blacklisted from "make format" because clang-format often destroys the readability of these tables.) https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:185: X(kNone, 7, kNone, "??") /* special condition / none */ Seems odd that kNone has the same encoding as LEZ? https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:186: //#define X(tag, opp, emit) By convention, this comment represents a prototype for the actual macro definitions that use this table, and as such, it should include all 4 parameters. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:368: void dump(const Cfg *Func) const override { (void)Func; }; Add a proper implementation of dump(). https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:828: if (Src0Ty == IceType_i64 || Src1Ty == IceType_i64) { Instead of testing both types against i64, I suggest something like: assert(Src0Ty == Src1Ty); if (Src0Ty == IceType_i64) { ... } Actually, you'll get a compiler warning/error in a NOASSERT build because Src1Ty is only used in the assert expression, so: assert(Src0Ty == CompareInst->getSrc(1)); https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:848: _br(TargetTrue, TargetFalse, DestT, NULL, CondMIPS32::Cond::EQZ); All the NULL should be nullptr. https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1499: if (!Instr.isDeleted() // only consider non-deleted instructions; and The isDeleted() check should be stronger. The whole "for" loop should start like this: for (Inst &Instr : Node->getInsts()) { if (Instr.isDeleted()) continue; ... } I think the x86 BoolFolding<Traits>::init() was changed to follow this form, but not before TargetARM32::ComputationTracker::recordProducers() cloned the original form. The consequence of the code here as written is that the FOREACH_VAR_IN_INST loop runs on deleted instructions and may inappropriately modify the bookkeeping. I'll fix the ARM code as a separate CL, because I'd like to carefully look at whether it makes any actual changes to the output. https://codereview.chromium.org/1993773004/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/cond-branch.ll (right): https://codereview.chromium.org/1993773004/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/cond-branch.ll:3: ; RUN: %if --need=allow_dump --need=target_MIPS32 --command %p2i --filetype=asm \ 80-col https://codereview.chromium.org/1993773004/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/cond-branch.ll:19: ; MIPS32: bne $a0, $a1, .Lcond_br_eq$branch2 I think you also need tests where branch folding is *not* done. E.g., this happens when the LLVM br instruction is not known to be the last use of the instruction's i1 operand. An easy way to trigger this is to do another RUN with -Om1 instead of -O2, because then liveness analysis is not run and so last-use info is not computed. Actually, it seems to me that the whole bool-folding prepass should be suppressed with Om1, but neither of the other targets are doing that, so it can be addressed separately.
https://codereview.chromium.org/1993773004/diff/20001/src/IceConditionCodesMI... File src/IceConditionCodesMIPS32.h (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceConditionCodesMI... src/IceConditionCodesMIPS32.h:1: //===- subzero/src/IceConditionCodesMIPS32.h - Condition Codes ---*- C++ On 2016/05/19 15:03:40, stichnot wrote: > Fix clang-format's line wrapping - probably change '---' to '--' and put > everything back onto a single line. Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:126: Operand *Src1, const InstMIPS32Label *Label, On 2016/05/19 15:03:40, stichnot wrote: > In ICE there's a pretty strong assumption/pattern that none of the instruction > src operands are nullptr. I think it would be cleaner (specifically for the > callers of this ctor) if you did one of two things: > > (1) Move Src0 and Src1 to the end of the arg list and make Src1 be an optional > parameter with a default of nullptr. > > (2) Have separate ctors for the 1 and 2 src arg cases. Personally I would > prefer this option. Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:131: if (Src1 != NULL) On 2016/05/19 15:03:40, stichnot wrote: > nullptr Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:328: return; On 2016/05/19 15:03:40, stichnot wrote: > this "return" is unnecessary, remove Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def File src/IceInstMIPS32.def (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:1: //===- subzero/src/IceInstMIPS32.def - X-Macros for MIPS32 insts --*- C++ -*-===// On 2016/05/19 15:03:41, stichnot wrote: > Fix this line to be 80-col - maybe "MIPS32 insts" should become "MIPS insts"? > Need to keep the magic "-* -C++ -*-" sequence for emacs, though it looks like > the space characters in that sequence might be optional. Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:175: /* enum value, opposite, emit */ \ On 2016/05/19 15:03:41, stichnot wrote: > The table has 4 columns, but this comment only documents 3 of them? > > Assuming this is modeled after the ARM table, the second column is supposed to > be the value used in the instruction encoding. > > If it's easier to just delete the encoding column for now, that's fine. Done. Deleted the encoding column. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:176: X(EQ, 0, NE, "eq") /* equal */ \ On 2016/05/19 15:03:40, stichnot wrote: > Optional: add extra spaces so the columns line up nicely. (Note that .def files > are blacklisted from "make format" because clang-format often destroys the > readability of these tables.) Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:185: X(kNone, 7, kNone, "??") /* special condition / none */ On 2016/05/19 15:03:41, stichnot wrote: > Seems odd that kNone has the same encoding as LEZ? Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.def#n... src/IceInstMIPS32.def:186: //#define X(tag, opp, emit) On 2016/05/19 15:03:40, stichnot wrote: > By convention, this comment represents a prototype for the actual macro > definitions that use this table, and as such, it should include all 4 > parameters. Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:368: void dump(const Cfg *Func) const override { (void)Func; }; On 2016/05/19 15:03:41, stichnot wrote: > Add a proper implementation of dump(). Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:828: if (Src0Ty == IceType_i64 || Src1Ty == IceType_i64) { On 2016/05/19 15:03:41, stichnot wrote: > Instead of testing both types against i64, I suggest something like: > assert(Src0Ty == Src1Ty); > if (Src0Ty == IceType_i64) { > ... > } > > Actually, you'll get a compiler warning/error in a NOASSERT build because Src1Ty > is only used in the assert expression, so: > assert(Src0Ty == CompareInst->getSrc(1)); Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:848: _br(TargetTrue, TargetFalse, DestT, NULL, CondMIPS32::Cond::EQZ); On 2016/05/19 15:03:41, stichnot wrote: > All the NULL should be nullptr. Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:888: return; On 2016/05/19 14:59:59, John wrote: > no need to "return" here. Done. https://codereview.chromium.org/1993773004/diff/20001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1499: if (!Instr.isDeleted() // only consider non-deleted instructions; and On 2016/05/19 15:03:41, stichnot wrote: > The isDeleted() check should be stronger. The whole "for" loop should start > like this: > for (Inst &Instr : Node->getInsts()) { > if (Instr.isDeleted()) > continue; > ... > } > > I think the x86 BoolFolding<Traits>::init() was changed to follow this form, but > not before TargetARM32::ComputationTracker::recordProducers() cloned the > original form. > > The consequence of the code here as written is that the FOREACH_VAR_IN_INST loop > runs on deleted instructions and may inappropriately modify the bookkeeping. > > I'll fix the ARM code as a separate CL, because I'd like to carefully look at > whether it makes any actual changes to the output. Done.
https://codereview.chromium.org/1993773004/diff/40001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:127: : InstMIPS32(Func, InstMIPS32::Br, 2, nullptr), TargetTrue(TargetTrue), s/2/1/ https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:822: // Since we don't we the producer of this boolean we will assume "we don't we" is wrong Also, can you reflow the comment to 80-col? https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1507: if (Dest // only instructions with an actual dest var; and I would start this comment as "only consider instructions..." https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/cond-branch.ll (right): https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/cond-branch.ll:26: ; MIPS32-OM1: xor $v0, $v0, $v1 You'll want to do something like: ; MIPS32-OM1-LABEL: cond_br_eq here and below, to guarantee the same level of test synchronization as the O2 tests. Another approach is to use two --check-prefix options for each test, one for the common patterns and one for this tests's specific patterns. E.g.: ; RUN: ... --check-prefix=COMMON --check-prefix=MIPS32 ; RUN: ... --check-prefix=COMMON --check-prefix=MIPS32-OM1 ; COMMON-LABEL: cond_br_eq ; MIPS32: bne ... ; MIPS32-OM1: xor ... https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/cond-branch.ll:41: ; MIPS32-OM1: xor $v0, $v0, $v1 Looking ahead, you may not want to hard-code register values quite so much into the tests. The issue is that if something later changes in the target lowering or in the register allocator, the register assignments might change and then you have to go and fix a bunch of "broken" tests. In tests like this on other targets, we often just limit checking to the opcode, unless perhaps there's a particular immediate value of interest.
https://codereview.chromium.org/1993773004/diff/40001/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/40001/src/IceInstMIPS32.cpp#n... src/IceInstMIPS32.cpp:127: : InstMIPS32(Func, InstMIPS32::Br, 2, nullptr), TargetTrue(TargetTrue), On 2016/05/23 19:53:05, stichnot wrote: > s/2/1/ Done. https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:822: // Since we don't we the producer of this boolean we will assume On 2016/05/23 19:53:05, stichnot wrote: > "we don't we" is wrong > > Also, can you reflow the comment to 80-col? Done. https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... src/IceTargetLoweringMIPS32.cpp:1507: if (Dest // only instructions with an actual dest var; and On 2016/05/23 19:53:05, stichnot wrote: > I would start this comment as "only consider instructions..." Done. https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/cond-branch.ll (right): https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/cond-branch.ll:26: ; MIPS32-OM1: xor $v0, $v0, $v1 On 2016/05/23 19:53:06, stichnot wrote: > You'll want to do something like: > > ; MIPS32-OM1-LABEL: cond_br_eq > > here and below, to guarantee the same level of test synchronization as the O2 > tests. > > Another approach is to use two --check-prefix options for each test, one for the > common patterns and one for this tests's specific patterns. E.g.: > > ; RUN: ... --check-prefix=COMMON --check-prefix=MIPS32 > ; RUN: ... --check-prefix=COMMON --check-prefix=MIPS32-OM1 > > ; COMMON-LABEL: cond_br_eq > ; MIPS32: bne ... > ; MIPS32-OM1: xor ... Done. https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/cond-branch.ll:41: ; MIPS32-OM1: xor $v0, $v0, $v1 On 2016/05/23 19:53:05, stichnot wrote: > Looking ahead, you may not want to hard-code register values quite so much into > the tests. The issue is that if something later changes in the target lowering > or in the register allocator, the register assignments might change and then you > have to go and fix a bunch of "broken" tests. > > In tests like this on other targets, we often just limit checking to the opcode, > unless perhaps there's a particular immediate value of interest. Done.
On 2016/05/24 05:44:36, sagar.thakur wrote: > https://codereview.chromium.org/1993773004/diff/40001/src/IceInstMIPS32.cpp > File src/IceInstMIPS32.cpp (right): > > https://codereview.chromium.org/1993773004/diff/40001/src/IceInstMIPS32.cpp#n... > src/IceInstMIPS32.cpp:127: : InstMIPS32(Func, InstMIPS32::Br, 2, nullptr), > TargetTrue(TargetTrue), > On 2016/05/23 19:53:05, stichnot wrote: > > s/2/1/ > > Done. > > https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... > File src/IceTargetLoweringMIPS32.cpp (right): > > https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:822: // Since we don't we the producer of this > boolean we will assume > On 2016/05/23 19:53:05, stichnot wrote: > > "we don't we" is wrong > > > > Also, can you reflow the comment to 80-col? > > Done. > > https://codereview.chromium.org/1993773004/diff/40001/src/IceTargetLoweringMI... > src/IceTargetLoweringMIPS32.cpp:1507: if (Dest // only instructions with an > actual dest var; and > On 2016/05/23 19:53:05, stichnot wrote: > > I would start this comment as "only consider instructions..." > > Done. > > https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... > File tests_lit/llvm2ice_tests/cond-branch.ll (right): > > https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/cond-branch.ll:26: ; MIPS32-OM1: xor $v0, $v0, $v1 > On 2016/05/23 19:53:06, stichnot wrote: > > You'll want to do something like: > > > > ; MIPS32-OM1-LABEL: cond_br_eq > > > > here and below, to guarantee the same level of test synchronization as the O2 > > tests. > > > > Another approach is to use two --check-prefix options for each test, one for > the > > common patterns and one for this tests's specific patterns. E.g.: > > > > ; RUN: ... --check-prefix=COMMON --check-prefix=MIPS32 > > ; RUN: ... --check-prefix=COMMON --check-prefix=MIPS32-OM1 > > > > ; COMMON-LABEL: cond_br_eq > > ; MIPS32: bne ... > > ; MIPS32-OM1: xor ... > > Done. > > https://codereview.chromium.org/1993773004/diff/40001/tests_lit/llvm2ice_test... > tests_lit/llvm2ice_tests/cond-branch.ll:41: ; MIPS32-OM1: xor $v0, $v0, $v1 > On 2016/05/23 19:53:05, stichnot wrote: > > Looking ahead, you may not want to hard-code register values quite so much > into > > the tests. The issue is that if something later changes in the target > lowering > > or in the register allocator, the register assignments might change and then > you > > have to go and fix a bunch of "broken" tests. > > > > In tests like this on other targets, we often just limit checking to the > opcode, > > unless perhaps there's a particular immediate value of interest. > > Done. Considering these two tests : 1. define internal i32 @cond_br_slt(i32 %arg1, i32 %arg2) { entry: %cmp1 = icmp slt i32 %arg1, %arg2 br i1 %cmp1, label %branch1, label %branch2 branch1: ret i32 1 branch2: ret i32 2 } O/P instructions : slt $a0, $a0, $a1 beqz $a0, .Lcond_br_slt$branch2 2. define internal i32 @cond_br_sle(i32 %arg1, i32 %arg2) { entry: %cmp1 = icmp sle i32 %arg1, %arg2 br i1 %cmp1, label %branch1, label %branch2 branch1: ret i32 1 branch2: ret i32 2 } O/P instructions : slt $a1, $a1, $a0 bnez $a1, .Lcond_br_sle$branch2 The order of operands for "slt" instruction is different for both tests. Therefore the detail of ordering of the operands will be left out if we only check the opcodes in the tests. Is it okay to leave out this detail?
lgtm https://codereview.chromium.org/1993773004/diff/60001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1993773004/diff/60001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:347: /// Create a conditional branch to the false node. Sorry, I forgot to comment on this last time. Usually the conditional branch is to the *true* node, with fallthrough to the *false* node. Are these forms of the branch instruction really meant to conditional-branch to the *false* node? (If this comment is actually wrong, it can be fixed as part of a future CL.)
Description was changed from ========== Added bool folding machinery for MIPS32. Added implementation for conditional branch instructions. ========== to ========== Added bool folding machinery for MIPS32. Added implementation for conditional branch instructions. R=jpp@chromium.org, 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:60001) manually as 5cce76190a9bd6a6a524b2afee917f6bec922d8d (presubmit successful).
Message was sent while issue was closed.
On 2016/05/24 06:59:02, sagar.thakur wrote:
> Considering these two tests :
>
> 1. define internal i32 @cond_br_slt(i32 %arg1, i32 %arg2) {
> entry:
> %cmp1 = icmp slt i32 %arg1, %arg2
> br i1 %cmp1, label %branch1, label %branch2
> branch1:
> ret i32 1
> branch2:
> ret i32 2
> }
>
> O/P instructions :
>
> slt $a0, $a0, $a1
> beqz $a0, .Lcond_br_slt$branch2
>
>
> 2. define internal i32 @cond_br_sle(i32 %arg1, i32 %arg2) {
> entry:
> %cmp1 = icmp sle i32 %arg1, %arg2
> br i1 %cmp1, label %branch1, label %branch2
> branch1:
> ret i32 1
> branch2:
> ret i32 2
> }
>
> O/P instructions :
>
> slt $a1, $a1, $a0
> bnez $a1, .Lcond_br_sle$branch2
>
> The order of operands for "slt" instruction is different for both tests.
> Therefore the detail of ordering of the operands will be left out if we only
> check the opcodes in the tests. Is it okay to leave out this detail?
I think this is fine.
The lit tests are more like smoke tests, in that most of their value is to catch
the introduction of major regressions.
The real value is in the cross tests (make -f Makefile.standalone check-xtest),
which compile and actually run an enormous set of unit tests.
Message was sent while issue was closed.
https://codereview.chromium.org/1993773004/diff/60001/src/IceInstMIPS32.h File src/IceInstMIPS32.h (right): https://codereview.chromium.org/1993773004/diff/60001/src/IceInstMIPS32.h#new... src/IceInstMIPS32.h:347: /// Create a conditional branch to the false node. On 2016/05/24 13:12:56, stichnot wrote: > Sorry, I forgot to comment on this last time. > > Usually the conditional branch is to the *true* node, with fallthrough to the > *false* node. Are these forms of the branch instruction really meant to > conditional-branch to the *false* node? > > (If this comment is actually wrong, it can be fixed as part of a future CL.) These branch instructions can be targeted to either the *true* node or the *false* node. I examined mips assembly output of llc and gcc. For code like this : if (foo == bar) a = foo; else a = bar; return a; The generated conditional branch is: bne foo, bar, %if.else And for code like this: if (foo != bar) a = foo; else a = bar; return a; The generated conditional branch is: beq foo, bar, %if.else From this I observed that both llc and gcc always emit conditional branches targeted to the else part (*false* node). And following the same approach as llc we are emitting conditional branches always targeted to the *false* node. Nevertheless we can also have the mips lowering to emit conditional branches to the *true* node, with fall through to the *false* node if it is more beneficial than emitting conditional branch to *false* node. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
