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

[Song, Ruiling]

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