[Beignet] [PATCH v2 1/2] GBE: Fix bug with negative constant GEP index.

[Zhigang Gong]

Right, I also noticed that unecessary loop in the original code. Will optimize it in a new patch.
Thanks for the review comments, just pushed.

On Mon, Dec 01, 2014 at 08:38:59AM +0000, Song, Ruiling wrote:
> The patch LGTM.
> As discussed, we need further cleanup for the code, the for loop below is not needed if it is not StructType.
> "for(int32_t ty_i=0; ty_i != TypeIndex; ty_i += step)"
> 
> > -----Original Message-----
> > From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> > Zhigang Gong
> > Sent: Wednesday, November 26, 2014 5:04 PM
> > To: beignet at lists.freedesktop.org
> > Cc: Gong, Zhigang
> > Subject: [Beignet] [PATCH v2 1/2] GBE: Fix bug with negative constant GEP
> > index.
> > 
> > GEP index may be negative constant value as below:
> > 
> > %arrayidx = getelementptr inbounds <4 x i32> addrspace(1)* %src4,
> > i32 %add.ptr.sum, i32 -4
> > 
> > The previous implementation assumes it's a unsigned value which is incorrect
> > and may cause infinite loop.
> > 
> > Signed-off-by: Zhigang Gong <zhigang.gong at intel.com>
> > ---
> >  backend/src/llvm/llvm_gen_backend.cpp |  5 +++--
> > backend/src/llvm/llvm_gen_backend.hpp |  2 +-
> >  backend/src/llvm/llvm_passes.cpp      | 17 +++++++++--------
> >  3 files changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/backend/src/llvm/llvm_gen_backend.cpp
> > b/backend/src/llvm/llvm_gen_backend.cpp
> > index a41c94c..3d74a0a 100644
> > --- a/backend/src/llvm/llvm_gen_backend.cpp
> > +++ b/backend/src/llvm/llvm_gen_backend.cpp
> > @@ -1126,7 +1126,7 @@ namespace gbe
> >        return pointer_reg;
> >      }
> >      else if (expr->getOpcode() == Instruction::GetElementPtr) {
> > -      uint32_t TypeIndex;
> > +      int32_t TypeIndex;
> >        uint32_t constantOffset = 0;
> > 
> >        Value *pointer = val;
> > @@ -1136,6 +1136,7 @@ namespace gbe
> >          ConstantInt* ConstOP =
> > dyn_cast<ConstantInt>(expr->getOperand(op));
> >          GBE_ASSERT(ConstOP);
> >          TypeIndex = ConstOP->getZExtValue();
> > +        GBE_ASSERT(TypeIndex >= 0);
> >          if (op == 1) {
> >            if (TypeIndex != 0) {
> >              Type *elementType =
> > (cast<PointerType>(pointer->getType()))->getElementType();
> > @@ -1145,7 +1146,7 @@ namespace gbe
> >              offset += elementSize * TypeIndex;
> >            }
> >          } else {
> > -          for(uint32_t ty_i=0; ty_i<TypeIndex; ty_i++)
> > +          for(int32_t ty_i=0; ty_i<TypeIndex; ty_i++)
> >            {
> >              Type* elementType = CompTy->getTypeAtIndex(ty_i);
> >              uint32_t align = getAlignmentByte(unit, elementType); diff
> > --git a/backend/src/llvm/llvm_gen_backend.hpp
> > b/backend/src/llvm/llvm_gen_backend.hpp
> > index 5dac3ea..ae0a818 100644
> > --- a/backend/src/llvm/llvm_gen_backend.hpp
> > +++ b/backend/src/llvm/llvm_gen_backend.hpp
> > @@ -66,7 +66,7 @@ namespace gbe
> >    static const OCLIntrinsicMap instrinsicMap;
> > 
> >    /*! Pad the offset */
> > -  uint32_t getPadding(uint32_t offset, uint32_t align);
> > +  int32_t getPadding(int32_t offset, int32_t align);
> > 
> >    /*! Get the type alignment in bytes */
> >    uint32_t getAlignmentByte(const ir::Unit &unit, llvm::Type* Ty); diff --git
> > a/backend/src/llvm/llvm_passes.cpp b/backend/src/llvm/llvm_passes.cpp
> > index f9fda4d..0f61526 100644
> > --- a/backend/src/llvm/llvm_passes.cpp
> > +++ b/backend/src/llvm/llvm_passes.cpp
> > @@ -126,7 +126,7 @@ namespace gbe
> >      return bKernel;
> >    }
> > 
> > -  uint32_t getPadding(uint32_t offset, uint32_t align) {
> > +  int32_t getPadding(int32_t offset, int32_t align) {
> >      return (align - (offset % align)) % align;
> >    }
> > 
> > @@ -279,32 +279,33 @@ namespace gbe
> > 
> >      for(uint32_t op=1; op<GEPInst->getNumOperands(); ++op)
> >      {
> > -      uint32_t TypeIndex;
> > +      int32_t TypeIndex;
> >        //we have a constant struct/array acces
> >        if(ConstantInt* ConstOP =
> > dyn_cast<ConstantInt>(GEPInst->getOperand(op)))
> >        {
> > -        uint32_t offset = 0;
> > +        int32_t offset = 0;
> >          TypeIndex = ConstOP->getZExtValue();
> > +        int32_t step = TypeIndex > 0 ? 1 : -1;
> >          if (op == 1) {
> >            if (TypeIndex != 0) {
> >              Type *elementType =
> > (cast<PointerType>(parentPointer->getType()))->getElementType();
> >              uint32_t elementSize = getTypeByteSize(unit, elementType);
> >              uint32_t align = getAlignmentByte(unit, elementType);
> >              elementSize += getPadding(elementSize, align);
> > -            offset += elementSize * TypeIndex;
> > +            offset += elementSize * TypeIndex * step;
> >            }
> >          } else {
> > -          for(uint32_t ty_i=0; ty_i<TypeIndex; ty_i++)
> > +          for(int32_t ty_i=0; ty_i != TypeIndex; ty_i += step)
> >            {
> >              Type* elementType = CompTy->getTypeAtIndex(ty_i);
> >              uint32_t align = getAlignmentByte(unit, elementType);
> > -            offset += getPadding(offset, align);
> > -            offset += getTypeByteSize(unit, elementType);
> > +            offset += getPadding(offset, align * step);
> > +            offset += getTypeByteSize(unit, elementType) * step;
> >            }
> > 
> >            //add getPaddingding for accessed type
> >            const uint32_t align = getAlignmentByte(unit,
> > CompTy->getTypeAtIndex(TypeIndex));
> > -          offset += getPadding(offset, align);
> > +          offset += getPadding(offset, align * step);
> >          }
> > 
> >          constantOffset += offset;
> > --
> > 1.8.3.2
> > 
> > _______________________________________________
> > 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