[Beignet] [PATCH] GBE: Support storing/loading pointers to/from private array
Song, Ruiling
ruiling.song at intel.com
Tue Jun 2 00:25:14 PDT 2015
Yes, I forget one minor case. That is when pointer operand is same pointer origin, I don't add it to pointerOrigMap.
I have sent V2 to fix the issue.
Thanks!
Ruiling
> -----Original Message-----
> From: Zhigang Gong [mailto:zhigang.gong at linux.intel.com]
> Sent: Monday, June 01, 2015 10:01 AM
> To: Song, Ruiling
> Cc: beignet at lists.freedesktop.org
> Subject: Re: [Beignet] [PATCH] GBE: Support storing/loading pointers
> to/from private array
>
> It seems that this patch cause one regression in the unit test cases.
> You can reproduce it as below:
>
> utests/utest_run compiler_local_slm
>
> Thanks,
> Zhigang Gong.
>
> On Thu, May 21, 2015 at 04:39:05PM +0800, Ruiling Song wrote:
> > The idea is create two additional array for holding pointer-base and
> > bti.
> >
> > Signed-off-by: Ruiling Song <ruiling.song at intel.com>
> > ---
> > backend/src/llvm/llvm_gen_backend.cpp | 210
> > ++++++++++++++++++++++++++++++++--
> > 1 file changed, 200 insertions(+), 10 deletions(-)
> >
> > diff --git a/backend/src/llvm/llvm_gen_backend.cpp
> > b/backend/src/llvm/llvm_gen_backend.cpp
> > index aec04fb..e5ba119 100644
> > --- a/backend/src/llvm/llvm_gen_backend.cpp
> > +++ b/backend/src/llvm/llvm_gen_backend.cpp
> > @@ -489,7 +489,7 @@ namespace gbe
> > map<Value *, Value *> BtiValueMap;
> > // map ptr to it's base
> > map<Value *, Value *> pointerBaseMap;
> > -
> > + std::set<Value *> addrStoreInst;
> > typedef map<Value *, Value *>::iterator PtrBaseMapIter;
> > /*! We visit each function twice. Once to allocate the registers and
> once to
> > * emit the Gen IR instructions
> > @@ -565,13 +565,14 @@ namespace gbe
> > BtiMap.clear();
> > BtiValueMap.clear();
> > pointerBaseMap.clear();
> > + addrStoreInst.clear();
> > // Reset for next function
> > btiBase = BTI_RESERVED_NUM;
> > return false;
> > }
> > /*! Given a possible pointer value, find out the interested escape like
> > load/store or atomic instruction */
> > - void findPointerEscape(Value *ptr, std::set<Value *> &mixedPtr, bool
> recordMixed);
> > + void findPointerEscape(Value *ptr, std::set<Value *> &mixedPtr,
> > + bool recordMixed, std::vector<Value *> &revisit);
> > /*! For all possible pointers, GlobalVariable, function pointer
> argument,
> > alloca instruction, find their pointer escape points */
> > void analyzePointerOrigin(Function &F); @@ -580,6 +581,8 @@
> > namespace gbe
> > bool isSingleBti(Value *Val);
> > Value *getBtiRegister(Value *v);
> > Value *getPointerBase(Value *ptr);
> > + void processPointerArray(Value *ptr, Value *bti, Value *base);
> > + void handleStoreLoadAddress(Function &F);
> >
> > MDNode *getKernelFunctionMetadata(Function *F);
> > virtual bool doFinalization(Module &M) { return false; } @@
> > -730,10 +733,13 @@ namespace gbe
> > return false;
> > }
> >
> > - void GenWriter::findPointerEscape(Value *ptr, std::set<Value *>
> > &mixedPtr, bool bFirstPass) {
> > + void GenWriter::findPointerEscape(Value *ptr, std::set<Value *>
> > + &mixedPtr, bool bFirstPass, std::vector<Value *> &revisit) {
> > std::vector<Value*> workList;
> > std::set<Value *> visited;
> > + // loadInst result maybe used as pointer
> > + std::set<LoadInst *> ptrCandidate;
> >
> > + bool isPointerArray = false;
> > if (ptr->use_empty()) return;
> >
> > workList.push_back(ptr);
> > @@ -741,7 +747,6 @@ namespace gbe
> > for (unsigned i = 0; i < workList.size(); i++) {
> > Value *work = workList[i];
> > if (work->use_empty()) continue;
> > -
> > for (Value::use_iterator iter = work->use_begin(); iter !=
> work->use_end(); ++iter) {
> > // After LLVM 3.5, use_iterator points to 'Use' instead of 'User',
> > // which is more straightforward.
> > @@ -750,6 +755,24 @@ namespace gbe
> > #else
> > User *theUser = iter->getUser();
> > #endif
> > + // becareful with sub operation
> > + if (isa<BinaryOperator>(theUser) &&
> dyn_cast<BinaryOperator>(theUser)->getOpcode() == Instruction::Sub) {
> > + // check both comes from ptrtoInt, don't need to traverse
> ptrdiff
> > + Value *op0 = theUser->getOperand(0);
> > + Value *op1 = theUser->getOperand(1);
> > + if ((isa<Instruction>(op0) &&
> dyn_cast<Instruction>(op0)->getOpcode() == Instruction::PtrToInt)
> > + &&(isa<Instruction>(op1) &&
> dyn_cast<Instruction>(op1)->getOpcode() == Instruction::PtrToInt)) {
> > + continue;
> > + }
> > + }
> > +
> > + if (isa<Instruction>(theUser)) {
> > + // some GlobalVariable maybe used in the function which is
> not current processed.
> > + // such kind of user should be skipped
> > + if
> (dyn_cast<Instruction>(theUser)->getParent()->getParent() != Func)
> > + continue;
> > + }
> > +
> > bool visitedInThisSource = visited.find(theUser) !=
> > visited.end();
> >
> > if (isa<SelectInst>(theUser) || isa<PHINode>(theUser)) @@
> > -797,8 +820,32 @@ namespace gbe
> > }
> >
> > // pointer address is used as the ValueOperand in store
> instruction, should be skipped
> > - if (StoreInst *load = dyn_cast<StoreInst>(theUser)) {
> > - if (load->getValueOperand() == work) {
> > + if (StoreInst *store = dyn_cast<StoreInst>(theUser)) {
> > + if (store->getValueOperand() == work) {
> > + addrStoreInst.insert(store);
> > + Value * pointerOperand = store->getPointerOperand();
> > + // check whether the pointerOperand already visited or
> not,
> > + // if not visited, then we need to record all the loadInst
> > + // on the origin of pointerOperand
> > + // if visited, that is the origin of the pointerOperand
> already
> > + // traversed, we need to the traverse again to record all
> the LoadInst
> > + PtrOrigMapIter pointerOpIter =
> pointerOrigMap.find(pointerOperand);
> > + bool pointerVisited = pointerOpIter !=
> pointerOrigMap.end();
> > + if (pointerVisited) {
> > + revisit.push_back((*pointerOpIter).second[0]);
> > + }
> > +
> > + PtrOrigMapIter ptrIter = pointerOrigMap.find(work);
> > + if (ptrIter == pointerOrigMap.end()) {
> > + // create new one
> > + SmallVector<Value *, 4> pointers;
> > + pointers.push_back(ptr);
> > + pointerOrigMap.insert(std::make_pair(work, pointers));
> > + } else {
> > + // update the pointer source here,
> > + (*ptrIter).second[0] = ptr;
> > + }
> > +
> > continue;
> > }
> > }
> > @@ -812,9 +859,15 @@ namespace gbe
> > }
> > Value *pointer = NULL;
> > if (isa<LoadInst>(theUser)) {
> > + ptrCandidate.insert(cast<LoadInst>(theUser));
> > pointer =
> dyn_cast<LoadInst>(theUser)->getPointerOperand();
> > } else if (isa<StoreInst>(theUser)) {
> > pointer =
> > dyn_cast<StoreInst>(theUser)->getPointerOperand();
> > + // Check whether we have stored a address to this pointer
> > + // if yes, we need to traverse the ptrCandidate, as they are
> loaded pointers
> > + if (addrStoreInst.find(theUser) != addrStoreInst.end()) {
> > + isPointerArray = true;
> > + }
> > } else if (isa<CallInst>(theUser)) {
> > // atomic/read(write)image
> > CallInst *ci = dyn_cast<CallInst>(theUser); @@ -843,7
> > +896,17 @@ namespace gbe
> > }
> > }
> > }
> > +
> > + if (isPointerArray) {
> > + GBE_ASSERT((isa<AllocaInst>(ptr) || ptrCandidate.empty())
> > + && "storing/loading pointers only support private
> array");
> > + for (auto x : ptrCandidate) {
> > + revisit.push_back(x);
> > + }
> > + }
> > + ptrCandidate.clear();
> > }
> > +
> > bool GenWriter::isSingleBti(Value *Val) {
> > // self + others same --> single
> > // all same ---> single
> > @@ -967,6 +1030,7 @@ namespace gbe
> > } else {
> > if (isSingleBti(Val)) {
> > PtrOrigMapIter iter = pointerOrigMap.find(Val);
> > + GBE_ASSERT(iter != pointerOrigMap.end());
> > Value * bti = getBtiRegister((*iter).second[0]);
> > BtiValueMap.insert(std::make_pair(Val, bti));
> > return bti;
> > @@ -976,6 +1040,7 @@ namespace gbe
> >
> > IRBuilder<> Builder(si->getParent());
> > PtrOrigMapIter iter = pointerOrigMap.find(Val);
> > + GBE_ASSERT(iter != pointerOrigMap.end());
> > Value *trueVal = getBtiRegister((*iter).second[0]);
> > Value *falseVal = getBtiRegister((*iter).second[1]);
> > Builder.SetInsertPoint(si); @@ -989,6 +1054,7 @@
> namespace
> > gbe
> >
> > PHINode *btiPhi =
> Builder.CreatePHI(IntegerType::get(Val->getContext(), 32),
> phi->getNumIncomingValues());
> > PtrOrigMapIter iter = pointerOrigMap.find(Val);
> > + GBE_ASSERT(iter != pointerOrigMap.end());
> > SmallVector<Value *, 4> &pointers = (*iter).second;
> > unsigned srcNum = pointers.size();
> > for (unsigned x = 0; x < srcNum; x++) { @@ -1105,6 +1171,91
> > @@ namespace gbe
> > }
> > }
> >
> > + void GenWriter::processPointerArray(Value *ptr, Value *bti, Value
> *base) {
> > + std::vector<Value*> workList;
> > + std::set<Value *> visited;
> > +
> > + if (ptr->use_empty()) return;
> > +
> > + workList.push_back(ptr);
> > +
> > + for (unsigned i = 0; i < workList.size(); i++) {
> > + Value *work = workList[i];
> > + if (work->use_empty()) continue;
> > +
> > + for (Value::use_iterator iter = work->use_begin(); iter !=
> work->use_end(); ++iter) {
> > + // After LLVM 3.5, use_iterator points to 'Use' instead of 'User',
> > + // which is more straightforward.
> > + #if (LLVM_VERSION_MAJOR == 3) && (LLVM_VERSION_MINOR < 5)
> > + User *theUser = *iter;
> > + #else
> > + User *theUser = iter->getUser(); #endif
> > + if(visited.find(theUser) != visited.end())
> > + continue;
> > +
> > + visited.insert(theUser);
> > +
> > + if (isa<LoadInst>(theUser) || isa<StoreInst>(theUser) ||
> isa<CallInst>(theUser)) {
> > + if (isa<CallInst>(theUser)) {
> > + Function *F =
> dyn_cast<CallInst>(theUser)->getCalledFunction();
> > + if (!F || F->getIntrinsicID() != 0) continue;
> > + }
> > + bool isLoad; Value *pointerOp;
> > +
> > + IRBuilder<> Builder(cast<Instruction>(theUser)->getParent());
> > + if (isa<LoadInst>(theUser)) {
> > + pointerOp =
> dyn_cast<LoadInst>(theUser)->getPointerOperand();
> > + isLoad = true;
> > + } else {
> > + pointerOp =
> dyn_cast<StoreInst>(theUser)->getPointerOperand();
> > + isLoad = false;
> > + }
> > + Builder.SetInsertPoint(cast<Instruction>(theUser));
> > +
> > + Type *int32Ty = Type::getInt32Ty(ptr->getContext());
> > + Value *v1 = Builder.CreatePtrToInt(pointerOp, int32Ty);
> > +
> > + PtrOrigMapIter originIter = pointerOrigMap.find(pointerOp);
> > + GBE_ASSERT(originIter != pointerOrigMap.end());
> > +
> > + Value *v2 = Builder.CreatePtrToInt(originIter->second[0],
> int32Ty);
> > + Value *v3 = Builder.CreatePtrToInt(base, int32Ty);
> > + Value *v4 = Builder.CreatePtrToInt(bti, int32Ty);
> > + // newLocBase = (pointer - origin) + base_start
> > + Value *diff = Builder.CreateSub(v1, v2);
> > + Value *newLocBase = Builder.CreateAdd(v3, diff);
> > + newLocBase = Builder.CreateIntToPtr(newLocBase,
> Type::getInt32PtrTy(ptr->getContext()));
> > + // newLocBti = (pointer - origin) + bti_start
> > + Value *newLocBti = Builder.CreateAdd(v4, diff);
> > + newLocBti = Builder.CreateIntToPtr(newLocBti,
> > + Type::getInt32PtrTy(ptr->getContext()));
> > +
> > + // later GenWriter instruction translation needs this map info
> > + BtiValueMap.insert(std::make_pair(newLocBti,
> ConstantInt::get(Type::getInt32Ty(ptr->getContext()), BTI_PRIVATE)));
> > + pointerBaseMap.insert(std::make_pair(newLocBti,
> > + ConstantPointerNull::get(cast<PointerType>(pointerOp->getType()))));
> > +
> > + BtiValueMap.insert(std::make_pair(newLocBase,
> ConstantInt::get(Type::getInt32Ty(ptr->getContext()), BTI_PRIVATE)));
> > + pointerBaseMap.insert(std::make_pair(newLocBase,
> > + ConstantPointerNull::get(cast<PointerType>(pointerOp->getType()))));
> > +
> > + if (isLoad) {
> > + Value *loadedBase = Builder.CreateLoad(newLocBase);
> > + Value *loadedBti = Builder.CreateLoad(newLocBti);
> > +
> > + BtiValueMap.insert(std::make_pair(theUser, loadedBti));
> > + pointerBaseMap.insert(std::make_pair(theUser,
> loadedBase));
> > + } else {
> > + Value *valueOp =
> cast<StoreInst>(theUser)->getValueOperand();
> > + Value *tmp =
> Builder.CreatePtrToInt(getPointerBase(valueOp),
> Type::getInt32Ty(ptr->getContext()));
> > + Builder.CreateStore(tmp, newLocBase);
> > + Builder.CreateStore(getBtiRegister(valueOp), newLocBti);
> > + }
> > + } else {
> > + workList.push_back(theUser);
> > + }
> > + }
> > + }
> > + }
> > +
> > void GenWriter::analyzePointerOrigin(Function &F) {
> > // used to record where the pointers get mixed (i.e. select or phi
> instruction)
> > std::set<Value *> mixedPtr;
> > @@ -1114,37 +1265,76 @@ namespace gbe
> > // and update the sources correctly. For pointers reachable from
> mixed-pointer, we will set
> > // its direct mixed-pointer parent as it's pointer origin.
> >
> > + std::vector<Value *> revisit;
> > // GlobalVariable
> > Module::GlobalListType &globalList =
> const_cast<Module::GlobalListType &> (TheModule->getGlobalList());
> > for(auto i = globalList.begin(); i != globalList.end(); i ++) {
> > GlobalVariable &v = *i;
> > if(!v.isConstantUsed()) continue;
> > - findPointerEscape(&v, mixedPtr, true);
> > + findPointerEscape(&v, mixedPtr, true, revisit);
> > }
> > // function argument
> > for (Function::arg_iterator I = F.arg_begin(), E = F.arg_end(); I != E;
> ++I) {
> > if (I->getType()->isPointerTy()) {
> > - findPointerEscape(I, mixedPtr, true);
> > + findPointerEscape(I, mixedPtr, true, revisit);
> > }
> > }
> > // alloca
> > BasicBlock &bb = F.getEntryBlock();
> > for (BasicBlock::iterator iter = bb.begin(), iterE = bb.end(); iter !=
> iterE; ++iter) {
> > if (AllocaInst *ai = dyn_cast<AllocaInst>(iter)) {
> > - findPointerEscape(ai, mixedPtr, true);
> > + findPointerEscape(ai, mixedPtr, true, revisit);
> > }
> > }
> > + // storing/loading pointer would introduce revisit
> > + for (std::vector<Value *>::iterator iter = revisit.begin(); iter !=
> revisit.end(); ++iter) {
> > + findPointerEscape(*iter, mixedPtr, true, revisit);
> > + }
> > +
> > // the second pass starts from mixed pointer
> > for (std::set<Value *>::iterator iter = mixedPtr.begin(); iter !=
> mixedPtr.end(); ++iter) {
> > - findPointerEscape(*iter, mixedPtr, false);
> > + findPointerEscape(*iter, mixedPtr, false, revisit);
> > }
> >
> > for (std::set<Value *>::iterator iter = mixedPtr.begin(); iter !=
> mixedPtr.end(); ++iter) {
> > getBtiRegister(*iter);
> > }
> > +
> > for (std::set<Value *>::iterator iter = mixedPtr.begin(); iter !=
> mixedPtr.end(); ++iter) {
> > getPointerBase(*iter);
> > }
> > + handleStoreLoadAddress(F);
> > + }
> > + void GenWriter::handleStoreLoadAddress(Function &F) {
> > + std::set<Value *> processed;
> > + for (std::set<Value *>::iterator iter = addrStoreInst.begin(); iter !=
> addrStoreInst.end(); ++iter) {
> > + StoreInst *store = cast<StoreInst>(*iter);
> > + Value *pointerOp = store->getPointerOperand();
> > + PtrOrigMapIter ptrIter = pointerOrigMap.find(pointerOp);
> > + GBE_ASSERT(ptrIter != pointerOrigMap.end());
> > + Value *base = (*ptrIter).second[0];
> > +
> > + if (processed.find(base) != processed.end()) {
> > + continue;
> > + }
> > + processed.insert(base);
> > +
> > + if (!isa<AllocaInst>(base)) continue;
> > +
> > + Value *ArraySize = cast<AllocaInst>(base)->getArraySize();
> > +
> > + BasicBlock &entry = F.getEntryBlock();
> > + BasicBlock::iterator bbIter = entry.begin();
> > + while (isa<AllocaInst>(bbIter)) ++bbIter;
> > +
> > + IRBuilder<> Builder(&entry);
> > + Builder.SetInsertPoint(bbIter);
> > +
> > + PointerType * AITy = cast<AllocaInst>(base)->getType();
> > + Value * btiArray = Builder.CreateAlloca(AITy->getElementType(),
> ArraySize, base->getName() + ".bti");
> > + Value * pointerBaseArray =
> Builder.CreateAlloca(AITy->getElementType(), ArraySize, base->getName() +
> ".pointer-base");
> > + processPointerArray(base, btiArray, pointerBaseArray);
> > + }
> > }
> >
> > void getSequentialData(const ConstantDataSequential *cda, void
> > *ptr, uint32_t &offset) {
> > --
> > 2.3.6
> >
> > _______________________________________________
> > Beignet mailing list
> > Beignet at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/beignet
More information about the Beignet
mailing list