Xiong hu, Good job. Some comments as below, The
On Tue, Aug 26, 2014 at 05:36:21AM +0800, [email protected] wrote: > From: Luo Xionghu <[email protected]> > > This regression is caused by structural analysis when check the if-then > node, acturally there are four types of if-then node according to the > topology and fallthrough information. fallthrough check is added in this > patch. > > Signed-off-by: Luo Xionghu <[email protected]> > --- > backend/src/backend/gen_insn_selection.cpp | 4 +++- > backend/src/ir/function.hpp | 5 +++++ > backend/src/ir/structural_analysis.cpp | 9 ++++++++- > backend/src/ir/structural_analysis.hpp | 16 +++++++++++++++- > 4 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/backend/src/backend/gen_insn_selection.cpp > b/backend/src/backend/gen_insn_selection.cpp > index b7a39af..9a552b1 100644 > --- a/backend/src/backend/gen_insn_selection.cpp > +++ b/backend/src/backend/gen_insn_selection.cpp > @@ -4018,7 +4018,9 @@ namespace gbe > sel.curr.physicalFlag = 0; > sel.curr.flagIndex = (uint64_t)pred; > sel.curr.externFlag = 1; > - sel.curr.inversePredicate = 1; > + if(insn.getParent()->need_reverse ){ > + sel.curr.inversePredicate = 1; > + } It's clearer to use sel.curr.inversePredicate = insn.getParent->need_reverse; > sel.curr.predicate = GEN_PREDICATE_NORMAL; > sel.IF(GenRegister::immd(0), jip, uip); > sel.curr.inversePredicate = 0; > diff --git a/backend/src/ir/function.hpp b/backend/src/ir/function.hpp > index c5582b4..b877bce 100644 > --- a/backend/src/ir/function.hpp > +++ b/backend/src/ir/function.hpp > @@ -87,6 +87,11 @@ namespace ir { > set <Register> definedPhiRegs; > /* these three are used by structure transforming */ > public: > + /*if need_reverse is true, need to reverse prediction. > + *if condition is TRUE, IF instruction will execute the following block, ~~~ should be * if, you missed one space between * and if > + * different from BRA instruction, so all the IF instruction need_reverse > + * except two special case(fallthrough is the same with succs.). */ > + bool need_reverse; > /* if needEndif is true, it means that this bb is the exit of an > * outermost structure, so this block needs another endif to match > * the if inserted at the entry of this structure, otherwise this > diff --git a/backend/src/ir/structural_analysis.cpp > b/backend/src/ir/structural_analysis.cpp > index dfc2118..c106fa7 100644 > --- a/backend/src/ir/structural_analysis.cpp > +++ b/backend/src/ir/structural_analysis.cpp > @@ -120,6 +120,7 @@ namespace analysis > /* since this node is an if node, so we remove the BRA instruction at > the bottom of the exit BB of 'node', > * and insert IF instead > */ > + pbb->need_reverse = node->need_reverse; It's better to add a new member to the IF(BRANCH) instruction to indicate this is a IF or an "inverse IF". Add a inversePredicate member to the branch instruction which is only valid for IF opcode and add a new method getInversePred() to that instruction and use that method function at the backend stage to check whether this is a IF NOT or IF Then. It's should be an instruction attribute rather than a basic block's. And please use inversePredicate and need_inverse rather than need_reverse to keep consistent with the backend's naming rule. Thanks Zhigang Gong _______________________________________________ Beignet mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/beignet
