[Beignet] [PATCH 2/3] GBE: fix the large if/endif block issue.
Yang, Rong R
rong.r.yang at intel.com
Mon Apr 28 21:04:47 PDT 2014
OK, this patchset LGTM.
-----Original Message-----
From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com]
Sent: Tuesday, April 29, 2014 10:40 AM
To: Yang, Rong R; Gong, Zhigang; beignet at lists.freedesktop.org
Cc: Gong, Zhigang
Subject: RE: [Beignet] [PATCH 2/3] GBE: fix the large if/endif block issue.
> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf
> Of Yang, Rong R
> Sent: Tuesday, April 29, 2014 10:15 AM
> To: Gong, Zhigang; beignet at lists.freedesktop.org
> Cc: Gong, Zhigang
> Subject: Re: [Beignet] [PATCH 2/3] GBE: fix the large if/endif block
issue.
>
> One comment.
>
> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf
> Of Zhigang Gong
> Sent: Monday, April 28, 2014 9:01 AM
> To: beignet at lists.freedesktop.org
> Cc: Gong, Zhigang
> Subject: [Beignet] [PATCH 2/3] GBE: fix the large if/endif block issue.
>
> Some test cases have some very large block which contains more than
32768/2
> instructions which could fit into one if/endif block.
>
> This patch introduce a ifendif fix switch at the GenContext.
> Once we encounter one of such error, we set the switch on and then
recompile
> the kernel. When the switch is on, we will insert extra endif/if pair
> to
the block
> to split one if/endif block to multiple ones to fix the large if/endif
issue.
>
> Signed-off-by: Zhigang Gong <zhigang.gong at intel.com>
> ---
> backend/src/backend/gen_context.cpp | 21 ++++++++++++++-------
> backend/src/backend/gen_context.hpp | 16 +++++++++++++++-
> backend/src/backend/gen_insn_selection.cpp | 19 ++++++++++++++++---
> backend/src/backend/gen_program.cpp | 8 ++++++++
> backend/src/backend/gen_reg_allocation.cpp | 9 ++++-----
> 5 files changed, 57 insertions(+), 16 deletions(-)
>
> diff --git a/backend/src/backend/gen_context.cpp
> b/backend/src/backend/gen_context.cpp
> index 349349d..64b822e 100644
> --- a/backend/src/backend/gen_context.cpp
> +++ b/backend/src/backend/gen_context.cpp
> @@ -52,6 +52,7 @@ namespace gbe
> this->p = NULL;
> this->sel = NULL;
> this->ra = NULL;
> + this->ifEndifFix = false;
> }
>
> GenContext::~GenContext(void) {
> @@ -72,6 +73,7 @@ namespace gbe
> this->ra = GBE_NEW(GenRegAllocator, *this);
> this->branchPos2.clear();
> this->branchPos3.clear();
> + this->errCode = NO_ERROR;
> }
>
> void GenContext::emitInstructionStream(void) { @@ -97,7 +99,7 @@
> namespace gbe
> p->NOP();
> }
>
> - void GenContext::patchBranches(void) {
> + bool GenContext::patchBranches(void) {
> using namespace ir;
> for (auto pair : branchPos2) {
> const LabelIndex label = pair.first; @@ -108,14 +110,17 @@
> namespace gbe
> for (auto pair : branchPos3) {
> const LabelPair labelPair = pair.first;
> const int32_t insnID = pair.second;
> - // FIXME the 'labelPair' implementation must be fixed, as it is
hard to
> - // convert InstructionSelection offset to ASM offset since asm
maybe
> compacted
> const int32_t jip = labelPos.find(labelPair.l0)->second;
> const int32_t uip = labelPos.find(labelPair.l1)->second;
> - assert((jip - insnID) < 32767 && (jip - insnID) > -32768);
> - assert((uip - insnID) < 32767 && (uip - insnID) > -32768);
> + if (((jip - insnID) > 32767 || (jip - insnID) < -32768) ||
> + ((uip - insnID) > 32768 || (uip - insnID) < -32768)) {
> + // The only possible error instruction is if/endif here.
> + errCode = OUT_OF_RANGE_IF_ENDIF;
> + return false;
> + }
> p->patchJMPI(insnID, (((uip - insnID)) << 16) | ((jip - insnID)));
> }
> + return true;
> }
>
> void GenContext::clearFlagRegister(void) { @@ -2013,7 +2018,8 @@
> namespace gbe
> this->clearFlagRegister();
> this->emitStackPointer();
> this->emitInstructionStream();
> - this->patchBranches();
> + if (this->patchBranches() == false)
> + return false;
> genKernel->insnNum = p->store.size();
> genKernel->insns = GBE_NEW_ARRAY_NO_ARG(GenInstruction,
> genKernel->insnNum);
> std::memcpy(genKernel->insns, &p->store[0], genKernel->insnNum *
> sizeof(GenInstruction)); @@ -2024,7 +2030,8 @@ namespace gbe
> GenNativeInstruction insn;
> std::cout << " L0:" << std::endl;
> for (uint32_t insnID = 0; insnID < genKernel->insnNum; ) {
> - if (labelPos.find((ir::LabelIndex)(curLabel + 1))->second ==
insnID) {
> + if (labelPos.find((ir::LabelIndex)(curLabel + 1))->second ==
insnID &&
> + curLabel < this->getFunction().labelNum()) {
> std::cout << " L" << curLabel + 1 << ":" << std::endl;
> curLabel = (ir::LabelIndex)(curLabel + 1);
> }
> diff --git a/backend/src/backend/gen_context.hpp
> b/backend/src/backend/gen_context.hpp
> index dfddd28..3b59797 100644
> --- a/backend/src/backend/gen_context.hpp
> +++ b/backend/src/backend/gen_context.hpp
> @@ -42,6 +42,13 @@ namespace gbe
> class SelectionInstruction; // Pre-RA Gen instruction
> class SelectionReg; // Pre-RA Gen register
> class GenRegister;
> + typedef enum {
> + NO_ERROR,
> + REGISTER_ALLOCATION_FAIL,
> + REGISTER_SPILL_EXCEED_THRESHOLD,
> + REGISTER_SPILL_FAIL,
> + OUT_OF_RANGE_IF_ENDIF,
> + } CompileErrorCode;
>
> /*! Context is the helper structure to build the Gen ISA or
> simulation
code
> * from GenIR
> @@ -73,7 +80,7 @@ namespace gbe
> /*! Emit the instructions */
> void emitInstructionStream(void);
> /*! Set the correct target values for the branches */
> - void patchBranches(void);
> + bool patchBranches(void);
> /*! Forward ir::Function isSpecialReg method */
> INLINE bool isSpecialReg(ir::Register reg) const {
> return fn.isSpecialReg(reg);
> @@ -177,12 +184,19 @@ namespace gbe
> uint32_t reservedSpillRegs;
> bool limitRegisterPressure;
> bool relaxMath;
> + const bool getIFENDIFFix(void) const { return ifEndifFix; }
> + void setIFENDIFFix(bool fix) { ifEndifFix = fix; }
> + const CompileErrorCode getErrCode() { return errCode; }
> private:
> + CompileErrorCode errCode;
> + bool ifEndifFix;
> /*! Build the curbe patch list for the given kernel */
> void buildPatchList(void);
> /*! allocate a new curbe register and insert to curbe pool. */
> void allocCurbeReg(ir::Register reg, gbe_curbe_type value,
> uint32_t subValue = 0);
>
> + friend GenRegAllocator; //!< need to access errCode
> directly.
> +
> };
>
> } /* namespace gbe */
> diff --git a/backend/src/backend/gen_insn_selection.cpp
> b/backend/src/backend/gen_insn_selection.cpp
> index 420737c..d484212 100644
> --- a/backend/src/backend/gen_insn_selection.cpp
> +++ b/backend/src/backend/gen_insn_selection.cpp
> @@ -196,7 +196,7 @@ namespace gbe
> // SelectionBlock
>
///////////////////////////////////////////////////////////////////////////
>
> - SelectionBlock::SelectionBlock(const ir::BasicBlock *bb) : bb(bb)
> {}
> + SelectionBlock::SelectionBlock(const ir::BasicBlock *bb) : bb(bb),
> + endifLabel( (ir::LabelIndex) 0){}
>
> void SelectionBlock::append(ir::Register reg) { tmp.push_back(reg);
> }
>
> @@ -975,7 +975,7 @@ namespace gbe
> this->LABEL(this->block->endifLabel);
> SelectionInstruction *insn = this->appendInsn(SEL_OP_ENDIF, 0, 1);
> insn->src(0) = src;
> - insn->index = uint16_t(jip);
> + insn->index = uint16_t(this->block->endifLabel);
> }
>
> void Selection::Opaque::CMP(uint32_t conditional, Reg src0, Reg
> src1,
Reg
> dst) { @@ -1476,13 +1476,26 @@ namespace gbe
> // If there is no branch at the end of this block.
>
> // Try all the patterns from best to worst
> -
> do {
> if ((*it)->emit(*this, dag))
> break;
> ++it;
> } while (it != end);
> GBE_ASSERT(it != end);
> + // If we are in if/endif fix mode, and this block is
> + // large enough, we need to insert endif/if pair to eliminate
> + // the too long if/endif block.
> + if (this->ctx.getIFENDIFFix() &&
> + this->block->insnList.size() != 0 &&
> + this->block->insnList.size() % 1000 == 0 &&
> + (uint16_t)this->block->endifLabel != 0) {
> >>>>>>>if needEndif is false, also need this Endif?
As you can see in the above condition check, This endif/if fix will be added only if "endifLabel != 0"
which means there is already an endif generated.
The needEndif bool variable in this function is just for those block which doesn't have a branch instruction and has no effect on if/endif fix logic.
More information about the Beignet
mailing list