[Beignet] [PATCH] GBE: fixed the out-of-range JMPI.

[Zhigang Gong]

nice catch, that's an oversight.
I just fixed it and pushed this patch.
Thanks.

On Tue, Jan 28, 2014 at 02:53:04AM +0000, Yang, Rong R wrote:
> One comment.
> 
> -----Original Message-----
> From: beignet-bounces at lists.freedesktop.org [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of Zhigang Gong
> Sent: Monday, January 27, 2014 5:22 PM
> To: beignet at lists.freedesktop.org
> Cc: Gong, Zhigang
> Subject: [Beignet] [PATCH] GBE: fixed the out-of-range JMPI.
> 
> For the conditional jump distance out of S15 range [-32768, 32767], we need to use an inverted jmp followed by a add ip, ip, distance to implement. A little hacky as we need to change the nop instruction to add instruction manually.
> 
> There is an optimization method which we can insert a ADD instruction on demand. But that will need some extra analysis for all the branching instruction. And need to adjust the distance for those branch instruction's start point and end point contains this instruction.
> 
> After this patch, the luxrender's slg4 could render the scene "alloy"
> correctly.
> 
> Signed-off-by: Zhigang Gong <zhigang.gong at intel.com>
> ---
>  backend/src/backend/gen_context.cpp |    6 +++---
>  backend/src/backend/gen_encoder.cpp |   30 +++++++++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/backend/src/backend/gen_context.cpp b/backend/src/backend/gen_context.cpp
> index 893de34..d30369d 100644
> --- a/backend/src/backend/gen_context.cpp
> +++ b/backend/src/backend/gen_context.cpp
> @@ -880,8 +880,8 @@ namespace gbe
>            NOT_IMPLEMENTED;
>          p->curr.execWidth = 1;
>          p->curr.noMask = 1;
> +        jip0 = p->n_instruction();
>          p->JMPI(GenRegister::immud(0));
> -        jip0 = p->n_instruction() - 1;
>        p->pop();
>  
>        p->curr.predicate = GEN_PREDICATE_NONE; @@ -905,8 +905,8 @@ namespace gbe
>            NOT_IMPLEMENTED;
>          p->curr.execWidth = 1;
>          p->curr.noMask = 1;
> +        jip1 = p->n_instruction();
>          p->JMPI(GenRegister::immud(0));
> -        jip1 = p->n_instruction() - 1;
>        p->pop();
>  
>        p->curr.predicate = GEN_PREDICATE_NONE; @@ -1457,7 +1457,7 @@ namespace gbe
>        p->curr.noMask = 1;
>        int jip = -(int)(p->n_instruction() - loop_start + 1) * 2;
>        p->JMPI(zero);
> -      p->patchJMPI(p->n_instruction()-1, jip);
> +      p->patchJMPI(p->n_instruction()-2, jip);
>        p->pop();
>        // end of loop
>      }
> diff --git a/backend/src/backend/gen_encoder.cpp b/backend/src/backend/gen_encoder.cpp
> index c372e36..1b79077 100644
> --- a/backend/src/backend/gen_encoder.cpp
> +++ b/backend/src/backend/gen_encoder.cpp
> @@ -1050,13 +1050,37 @@ namespace gbe
>  
>    void GenEncoder::JMPI(GenRegister src) {
>      alu2(this, GEN_OPCODE_JMPI, GenRegister::ip(), GenRegister::ip(), src);
> +    NOP();
>    }
>  
>    void GenEncoder::patchJMPI(uint32_t insnID, int32_t jumpDistance) {
>      GenInstruction &insn = this->store[insnID];
> -    assert(insnID < this->store.size());
> -    assert(insn.header.opcode == GEN_OPCODE_JMPI);
> -    this->setSrc1(&insn, GenRegister::immd(jumpDistance));
> +    GBE_ASSERT(insnID < this->store.size());
> +    GBE_ASSERT(insn.header.opcode == GEN_OPCODE_JMPI);
> +    if ( (jumpDistance > -32767 && jumpDistance < 32768)
> +         || (insn.header.predicate_control == GEN_PREDICATE_NONE)) {
> 
> >>>>>> If predicate_control is GEN_PREDICATE_NONE and jumpDistance is out of range, also can't use jmpi.
> 
> +        this->setSrc1(&insn, GenRegister::immd(jumpDistance));
> +    } else {
> +      // For the conditional jump distance out of S15 range, we need to use an
> +      // inverted jmp followed by a add ip, ip, distance to implement.
> +      // A little hacky as we need to change the nop instruction to add
> +      // instruction manually.
> +      // FIXME there is an optimization method which we can insert a
> +      // ADD instruction on demand. But that will need some extra analysis
> +      // for all the branching instruction. And need to adjust the distance
> +      // for those branch instruction's start point and end point contains
> +      // this instruction.
> +      insn.header.predicate_inverse ^= 1;
> +      this->setSrc1(&insn, GenRegister::immd(2));
> +      GenInstruction &insn2 = this->store[insnID+1];
> +      GBE_ASSERT(insn2.header.opcode == GEN_OPCODE_NOP);
> +      GBE_ASSERT(insnID < this->store.size());
> +      insn2.header.predicate_control = GEN_PREDICATE_NONE;
> +      insn2.header.opcode = GEN_OPCODE_ADD;
> +      this->setDst(&insn2, GenRegister::ip());
> +      this->setSrc0(&insn2, GenRegister::ip());
> +      this->setSrc1(&insn2, GenRegister::immd(jumpDistance * 8));
> +    }
>    }
>  
>    void GenEncoder::CMP(uint32_t conditional, GenRegister src0, GenRegister src1) {
> --
> 1.7.9.5
> 
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/beignet