[polly] r248603 - Use per-Purpose overloads for MemoryAccess creation

Johannes Doerfert via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 25 16:46:33 PDT 2015


Idea and patch LGTM, some comments that you might want to address are
inlined.

> Author: meinersbur
> Date: Fri Sep 25 13:53:27 2015
> New Revision: 248603
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=248603&view=rev
> Log:
> Use per-Purpose overloads for MemoryAccess creation
> 
> This makes the intent of each created object clearer and allows to add more specific asserts. The bug fixed in r248535 has been discovered this way.
> 
> No functional change intended; everything should behave as before.
> 
> Modified:
>     polly/trunk/include/polly/ScopInfo.h
>     polly/trunk/lib/Analysis/ScopInfo.cpp
> 
> Modified: polly/trunk/include/polly/ScopInfo.h
> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/include/polly/ScopInfo.h?rev=248603&r1=248602&r2=248603&view=diff
> ==============================================================================
> --- polly/trunk/include/polly/ScopInfo.h (original)
> +++ polly/trunk/include/polly/ScopInfo.h Fri Sep 25 13:53:27 2015
> @@ -1490,22 +1490,60 @@ class ScopInfo : public RegionPass {
>                         ArrayRef<const SCEV *> Subscripts,
>                         ArrayRef<const SCEV *> Sizes, bool IsPHI);
>  
> -  void addMemoryAccess(BasicBlock *BB, Instruction *Inst,
> -                       MemoryAccess::AccessType Type, Value *BaseAddress,
> -                       unsigned ElemBytes, bool Affine, Value *AccessValue,
> -                       bool IsPHI = false) {
> -    addMemoryAccess(BB, Inst, Type, BaseAddress, ElemBytes, Affine, AccessValue,
> -                    ArrayRef<const SCEV *>(), ArrayRef<const SCEV *>(), IsPHI);
> -  }
> -
> -  void addMemoryAccess(BasicBlock *BB, Instruction *Inst,
> -                       MemoryAccess::AccessType Type, Value *BaseAddress,
> -                       unsigned ElemBytes, bool Affine,
> -                       ArrayRef<const SCEV *> Subscripts,
> -                       ArrayRef<const SCEV *> Sizes, Value *AccessValue) {
> -    addMemoryAccess(BB, Inst, Type, BaseAddress, ElemBytes, Affine, AccessValue,
> -                    Subscripts, Sizes, false);
> -  }
> +  /// @brief Create a MemoryAccess that represents either a LoadInst or
> +  /// StoreInst.
> +  ///
> +  /// @param MemAccInst  The LoadInst or StoreInst.
> +  /// @param Type        The kind of access.
> +  /// @param BaseAddress The accessed array's base address.
> +  /// @param ElemBytes   Size of accessed array element.
> +  /// @param IsAffine    Whether all subscripts are affine expressions.
> +  /// @param Subscripts  Access subscripts per dimension.
> +  /// @param Sizes       The array dimension's sizes.
> +  /// @param AccessValue Value read or written.
> +  void addExplicitAccess(Instruction *MemAccInst, MemoryAccess::AccessType Type,
> +                         Value *BaseAddress, unsigned ElemBytes, bool IsAffine,
> +                         ArrayRef<const SCEV *> Subscripts,
> +                         ArrayRef<const SCEV *> Sizes, Value *AccessValue);
Could we call this a MemoryAccess in contrast to ScalarAccesses? Hence,
  Create an Access for a memory load or store.

> +  /// @brief Create a MemoryAccess for writing to .s2a memory.
> +  ///
> +  /// The access will be created at the @p Value's definition.
> +  ///
> +  /// @param Value The value to be written into the .s2a memory.
> +  void addScalarWriteAccess(Instruction *Value);
I would avoid .s2a as it is an internal artifact. Maybe just say
something like:
  Create a write Access for the definition of a sclar.

> +  /// @brief Create a MemoryAccess for reading from .s2a memory.
> +  ///
> +  /// @param Value The scalar expected to be loaded.
> +  /// @param User  User of the scalar, where the access is added.
> +  void addScalarReadAccess(Value *Value, Instruction *User);
Similar to above.

> +  /// @brief Create a MemoryAccess for reading from .s2a memory.
Similar to above.

> +  /// This is for PHINodes using the scalar. It is used when leaving the
> +  /// incoming block to write to the .phiops location.
> +  ///
> +  /// @param Value  The scalar expected to be loaded.
> +  /// @param User   The PHI node referencing @p Value.
> +  /// @param UserBB Incoming block for the incoming @p Value.
> +  void addScalarReadAccess(Value *Value, PHINode *User, BasicBlock *UserBB);
Maybe we could name this differently to make the purpose clearer, e.g.,
  addPHIOpReadAccess

> +  /// @brief Create a MemoryAccess for writing to .phiops memory.
> +  ///
> +  /// @param PHI           PHINode under consideration.
> +  /// @param IncomingBlock Some predecessor block.
> +  /// @param IncomingValue @p PHI's value when coming from @p IncomingBlock.
> +  /// @param IsExitBlock   When true, use the .s2a alloca instead. Used for
> +  ///                      values escaping through a PHINode in the SCoP
> +  ///                      region's exit block.
> +  void addPHIWriteAccess(PHINode *PHI, BasicBlock *IncomingBlock,
> +                         Value *IncomingValue, bool IsExitBlock);
> +
> +  /// @brief Create a MemoryAccess for reading from .phiops memory.
> +  ///
> +  /// @param PHI PHINode under consideration. READ access will be added here.
> +  void addPHIReadAccess(PHINode *PHI);
Again avoid .phiops and make it something like: the phi operand

>  public:
>    static char ID;
> 
> Modified: polly/trunk/lib/Analysis/ScopInfo.cpp
> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopInfo.cpp?rev=248603&r1=248602&r2=248603&view=diff
> ==============================================================================
> --- polly/trunk/lib/Analysis/ScopInfo.cpp (original)
> +++ polly/trunk/lib/Analysis/ScopInfo.cpp Fri Sep 25 13:53:27 2015
> @@ -2703,26 +2703,18 @@ void ScopInfo::buildPHIAccesses(PHINode
>        // we have to insert a scalar dependence from the definition of OpI to
>        // OpBB if the definition is not in OpBB.
>        if (OpIBB != OpBB) {
> -        addMemoryAccess(OpBB, PHI, MemoryAccess::READ, OpI, 1, true, OpI);
> -        addMemoryAccess(OpIBB, OpI, MemoryAccess::MUST_WRITE, OpI, 1, true,
> -                        OpI);
> +        addScalarReadAccess(OpI, PHI, OpBB);
> +        addScalarWriteAccess(OpI);
>        }
>      } else if (ModelReadOnlyScalars && !isa<Constant>(Op)) {
> -      addMemoryAccess(OpBB, PHI, MemoryAccess::READ, Op, 1, true, Op);
> +      addScalarReadAccess(Op, PHI, OpBB);
>      }
>  
> -    // Always use the terminator of the incoming basic block as the access
> -    // instruction.
> -    OpI = OpBB->getTerminator();
> -
> -    addMemoryAccess(OpBB, OpI, MemoryAccess::MUST_WRITE, PHI, 1, true, Op,
> -                    /* IsPHI */ !IsExitBlock);
> +    addPHIWriteAccess(PHI, OpBB, Op, IsExitBlock);
>    }
>  
> -  if (!OnlyNonAffineSubRegionOperands) {
> -    addMemoryAccess(PHI->getParent(), PHI, MemoryAccess::READ, PHI, 1, true,
> -                    PHI,
> -                    /* IsPHI */ !IsExitBlock);
> +  if (!OnlyNonAffineSubRegionOperands && !IsExitBlock) {
> +    addPHIReadAccess(PHI);
>    }
>  }
>  
> @@ -2778,7 +2770,7 @@ bool ScopInfo::buildScalarDependences(In
>      // Do not build a read access that is not in the current SCoP
>      // Use the def instruction as base address of the MemoryAccess, so that it
>      // will become the name of the scalar access in the polyhedral form.
> -    addMemoryAccess(UseParent, UI, MemoryAccess::READ, Inst, 1, true, Inst);
> +    addScalarReadAccess(Inst, UI);
>    }
>  
>    if (ModelReadOnlyScalars && !isa<PHINode>(Inst)) {
> @@ -2793,8 +2785,7 @@ bool ScopInfo::buildScalarDependences(In
>        if (isa<Constant>(Op))
>          continue;
>  
> -      addMemoryAccess(Inst->getParent(), Inst, MemoryAccess::READ, Op, 1, true,
> -                      Op);
> +      addScalarReadAccess(Op, Inst);
>      }
>    }
>  
> @@ -2865,8 +2856,8 @@ void ScopInfo::buildMemoryAccess(
>          SizesSCEV.push_back(SE->getSCEV(ConstantInt::get(
>              IntegerType::getInt64Ty(BasePtr->getContext()), Size)));
>  
> -        addMemoryAccess(Inst->getParent(), Inst, Type, BasePointer->getValue(),
> -                        Size, true, Subscripts, SizesSCEV, Val);
> +        addExplicitAccess(Inst, Type, BasePointer->getValue(), Size, true,
> +                          Subscripts, SizesSCEV, Val);
>          return;
>        }
>      }
> @@ -2874,9 +2865,9 @@ void ScopInfo::buildMemoryAccess(
>  
>    auto AccItr = InsnToMemAcc.find(Inst);
>    if (PollyDelinearize && AccItr != InsnToMemAcc.end()) {
> -    addMemoryAccess(Inst->getParent(), Inst, Type, BasePointer->getValue(),
> -                    Size, true, AccItr->second.DelinearizedSubscripts,
> -                    AccItr->second.Shape->DelinearizedSizes, Val);
> +    addExplicitAccess(Inst, Type, BasePointer->getValue(), Size, true,
> +                      AccItr->second.DelinearizedSubscripts,
> +                      AccItr->second.Shape->DelinearizedSizes, Val);
>      return;
>    }
>  
> @@ -2893,15 +2884,16 @@ void ScopInfo::buildMemoryAccess(
>    bool IsAffine = !isVariantInNonAffineLoop &&
>                    isAffineExpr(R, AccessFunction, *SE, BasePointer->getValue());
>  
> -  SmallVector<const SCEV *, 4> Subscripts, Sizes;
> -  Subscripts.push_back(AccessFunction);
> -  Sizes.push_back(SE->getConstant(ZeroOffset->getType(), Size));
> +  // FIXME: Size if the number of bytes of an array element, not the number of
> +  // elements as probably intended here.
What exactly does this FIXME mean and why is it added?

> +  const SCEV *SizeSCEV = SE->getConstant(ZeroOffset->getType(), Size);
>  
>    if (!IsAffine && Type == MemoryAccess::MUST_WRITE)
>      Type = MemoryAccess::MAY_WRITE;
>  
> -  addMemoryAccess(Inst->getParent(), Inst, Type, BasePointer->getValue(), Size,
> -                  IsAffine, Subscripts, Sizes, Val);
> +  addExplicitAccess(Inst, Type, BasePointer->getValue(), Size, IsAffine,
> +                    ArrayRef<const SCEV *>(AccessFunction),
> +                    ArrayRef<const SCEV *>(SizeSCEV), Val);
>  }
>  
>  void ScopInfo::buildAccessFunctions(Region &R, Region &SR) {
> @@ -2946,8 +2938,7 @@ void ScopInfo::buildAccessFunctions(Regi
>  
>      if (buildScalarDependences(Inst, &R, NonAffineSubRegion)) {
>        if (!isa<StoreInst>(Inst))
> -        addMemoryAccess(&BB, Inst, MemoryAccess::MUST_WRITE, Inst, 1, true,
> -                        Inst);
> +        addScalarWriteAccess(Inst);
>      }
>    }
>  }
> @@ -2957,8 +2948,7 @@ void ScopInfo::addMemoryAccess(BasicBloc
>                                 Value *BaseAddress, unsigned ElemBytes,
>                                 bool Affine, Value *AccessValue,
>                                 ArrayRef<const SCEV *> Subscripts,
> -                               ArrayRef<const SCEV *> Sizes,
> -                               bool IsPHI = false) {
> +                               ArrayRef<const SCEV *> Sizes, bool IsPHI) {
>    AccFuncSetType &AccList = AccFuncMap[BB];
>    size_t Identifier = AccList.size();
>  
> @@ -2972,6 +2962,43 @@ void ScopInfo::addMemoryAccess(BasicBloc
>                         Subscripts, Sizes, AccessValue, IsPHI, BaseName);
>  }
>  
> +void ScopInfo::addExplicitAccess(
> +    Instruction *MemAccInst, MemoryAccess::AccessType Type, Value *BaseAddress,
> +    unsigned ElemBytes, bool IsAffine, ArrayRef<const SCEV *> Subscripts,
> +    ArrayRef<const SCEV *> Sizes, Value *AccessValue) {
> +  assert(isa<LoadInst>(MemAccInst) || isa<StoreInst>(MemAccInst));
> +  assert(isa<LoadInst>(MemAccInst) == (Type == MemoryAccess::READ));
> +  addMemoryAccess(MemAccInst->getParent(), MemAccInst, Type, BaseAddress,
> +                  ElemBytes, IsAffine, AccessValue, Subscripts, Sizes, false);
> +}
> +void ScopInfo::addScalarWriteAccess(Instruction *Value) {
> +  addMemoryAccess(Value->getParent(), Value, MemoryAccess::MUST_WRITE, Value, 1,
> +                  true, Value, ArrayRef<const SCEV *>(),
> +                  ArrayRef<const SCEV *>(), false);
> +}
> +void ScopInfo::addScalarReadAccess(Value *Value, Instruction *User) {
> +  assert(!isa<PHINode>(User));
> +  addMemoryAccess(User->getParent(), User, MemoryAccess::READ, Value, 1, true,
> +                  Value, ArrayRef<const SCEV *>(), ArrayRef<const SCEV *>(),
> +                  false);
> +}
> +void ScopInfo::addScalarReadAccess(Value *Value, PHINode *User,
> +                                   BasicBlock *UserBB) {
> +  addMemoryAccess(UserBB, User, MemoryAccess::READ, Value, 1, true, Value,
> +                  ArrayRef<const SCEV *>(), ArrayRef<const SCEV *>(), false);
> +}
> +void ScopInfo::addPHIWriteAccess(PHINode *PHI, BasicBlock *IncomingBlock,
> +                                 Value *IncomingValue, bool IsExitBlock) {
> +  addMemoryAccess(IncomingBlock, IncomingBlock->getTerminator(),
> +                  MemoryAccess::MUST_WRITE, PHI, 1, true, IncomingValue,
> +                  ArrayRef<const SCEV *>(), ArrayRef<const SCEV *>(),
> +                  !IsExitBlock);
> +}
> +void ScopInfo::addPHIReadAccess(PHINode *PHI) {
> +  addMemoryAccess(PHI->getParent(), PHI, MemoryAccess::READ, PHI, 1, true, PHI,
> +                  ArrayRef<const SCEV *>(), ArrayRef<const SCEV *>(), true);
> +}
> +
>  Scop *ScopInfo::buildScop(Region &R, DominatorTree &DT) {
>    unsigned MaxLoopDepth = getMaxLoopDepthInRegion(R, *LI, *SD);
>    Scop *S = new Scop(R, AccFuncMap, *SE, DT, ctx, MaxLoopDepth);
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.31

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150926/43601328/attachment.sig>


More information about the llvm-commits mailing list