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:[email protected]] On Behalf Of > > Zhigang Gong > > Sent: Wednesday, November 26, 2014 5:04 PM > > To: [email protected] > > 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 <[email protected]> > > --- > > 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 > > [email protected] > > http://lists.freedesktop.org/mailman/listinfo/beignet > _______________________________________________ > Beignet mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/beignet _______________________________________________ Beignet mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/beignet
