[polly] delinearize together all array refs

[Tobias Grosser]

On 09/05/2014 23:07, Sebastian Pop wrote:
> Hi Tobi,
>
> here is the first half of making polly delinearize all memory accesses together:
> this is only modifying the scop detection. I wanted to get your comments on this
> part before I do the next part in the translation to the polyhedral representation.

Nice. I like this small patch. I have some comments inline.

>  From e772e224cb6dd24a0555fadf8d78d3309b6102b9 Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Thu, 8 May 2014 11:03:02 -0500
> Subject: [PATCH] delinearize together all accesses to the same array
>
> ---
>   include/polly/ScopDetection.h  |   15 ++++++++++
>   lib/Analysis/ScopDetection.cpp |   58 ++++++++++++++++++++++++++++++++-------
>   2 files changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/include/polly/ScopDetection.h b/include/polly/ScopDetection.h
> index 452eb09..1fd20bd 100644
> --- a/include/polly/ScopDetection.h
> +++ b/include/polly/ScopDetection.h
> @@ -63,6 +63,7 @@ class Loop;
>   class ScalarEvolution;
>   class SCEV;
>   class SCEVAddRecExpr;
> +class SCEVUnknown;
>   class CallInst;
>   class Instruction;
>   class AliasAnalysis;
> @@ -96,6 +97,15 @@ class ScopDetection : public FunctionPass {
>       Region &CurRegion;   // The region to check.
>       AliasSetTracker AST; // The AliasSetTracker to hold the alias information.
>       bool Verifying;      // If we are in the verification phase?
> +    // The set of base pointers for non affine array accesses.
> +    std::set<const SCEVUnknown *> NonAffineArrays;

This set seems redundant. Any reason you can not use
NonAffineAccesses.count() to check if a base pointer

> +
> +    typedef std::vector<const SCEVAddRecExpr *> AFs;
> +    typedef std::map<const SCEVUnknown *, AFs> BaseToAFs;
> +
> +    // Map a base pointer to all access functions accessing it.
> +    BaseToAFs NonAffineAccesses;
> +
>       DetectionContext(Region &R, AliasAnalysis &AA, bool Verify)
>           : CurRegion(R), AST(AA), Verifying(Verify) {}
>     };
> @@ -112,6 +122,11 @@ class ScopDetection : public FunctionPass {
>     FunctionSet InvalidFunctions;
>     mutable std::string LastFailure;
>
> +  // Delinearize all non affine memory accesses and returns true when there
         Delinearize                                    returns

         Either imperative or third person singular, but not mixed.

> +  // exists a non affine memory access that cannot be delinearized. Returns
 >
> +  // false when all array accesses are affine after delinearization.
> +  bool hasNonAffineMemoryAccesses(DetectionContext &Context) const;
> +
>     // Try to expand the region R. If R can be expanded return the expanded
>     // region, NULL otherwise.
>     Region *expandRegion(Region &R);
> diff --git a/lib/Analysis/ScopDetection.cpp b/lib/Analysis/ScopDetection.cpp
> index 62b9c2a..494189e 100644
> --- a/lib/Analysis/ScopDetection.cpp
> +++ b/lib/Analysis/ScopDetection.cpp
> @@ -337,6 +337,34 @@ bool ScopDetection::isInvariant(const Value &Val, const Region &Reg) const {
>     return true;
>   }
>
> +bool
> +ScopDetection::hasNonAffineMemoryAccesses(DetectionContext &Context) const {
> +  for (const SCEVUnknown *BasePointer : Context.NonAffineArrays) {
> +    Value *BaseValue = BasePointer->getValue();
> +
> +    // First step: collect parametric terms in all array references.
> +    SmallVector<const SCEV *, 4> Terms;
> +    for (const SCEVAddRecExpr *AF : Context.NonAffineAccesses[BasePointer])
> +      AF->collectParametricTerms(*SE, Terms);
> +
> +    // Second step: find array shape.
> +    SmallVector<const SCEV *, 4> Sizes;
> +    SE->findArrayDimensions(Terms, Sizes);
> +
> +    // Third step: compute the access functions for each subscript.
> +    for (const SCEVAddRecExpr *AF : Context.NonAffineAccesses[BasePointer]) {
> +      SmallVector<const SCEV *, 4> Subscripts;
> +      AF->computeAccessFunctions(*SE, Subscripts, Sizes);
> +
> +      // Check that the delinearized subscripts are affine.
> +      for (const SCEV *S : Subscripts)
> +        if (!isAffineExpr(&Context.CurRegion, S, *SE, BaseValue))
> +          return invalid<ReportNonAffineAccess>(Context, /*Assert=*/true, AF);
> +    }
> +  }
> +  return false;
> +}
> +
>   bool ScopDetection::isValidMemoryAccess(Instruction &Inst,
>                                           DetectionContext &Context) const {
>     Value *Ptr = getPointerOperand(Inst);
> @@ -370,21 +398,22 @@ bool ScopDetection::isValidMemoryAccess(Instruction &Inst,
>     } else if (!isAffineExpr(&Context.CurRegion, AccessFunction, *SE,
>                              BaseValue)) {
>       const SCEVAddRecExpr *AF = dyn_cast<SCEVAddRecExpr>(AccessFunction);
> +
>       if (!PollyDelinearize || !AF)
>         return invalid<ReportNonAffineAccess>(Context, /*Assert=*/true,
>                                               AccessFunction);
>
> -    // Try to delinearize AccessFunction only when the expression is known to
> -    // not be affine: as all affine functions can be represented without
> -    // problems in Polly, we do not have to delinearize them.
> -    SmallVector<const SCEV *, 4> Subscripts, Sizes;
> -    AF->delinearize(*SE, Subscripts, Sizes);
> -    int size = Subscripts.size();
> -
> -    for (int i = 0; i < size; ++i)
> -      if (!isAffineExpr(&Context.CurRegion, Subscripts[i], *SE, BaseValue))
> -        return invalid<ReportNonAffineAccess>(Context, /*Assert=*/true,
> -                                              AccessFunction);
> +    // Collect all non affine memory accesses, and check whether they are linear
> +    // at the end of scop detection. That way we can delinearize all the memory
> +    // accesses to the same array in a unique step.
> +    if (Context.NonAffineArrays.count(BasePointer) == 0) {
> +      Context.NonAffineArrays.insert(BasePointer);
> +
> +      typedef std::vector<const SCEVAddRecExpr *> AFs;

Instead of introducing another typedef, would it make sense to reuse the 
one defined in the header?

> +      Context.NonAffineAccesses[BasePointer] = AFs();
> +    }
> +    Context.NonAffineAccesses[BasePointer].push_back(AF);
 >
>     }
>
>     // FIXME: Alias Analysis thinks IntToPtrInst aliases with alloca instructions
> @@ -604,11 +633,18 @@ bool ScopDetection::allBlocksValid(DetectionContext &Context) const {
>       if (!isValidCFG(*BB, Context))
>         return false;
>
> +  // Clear before starting collecting non affine accesses in the current region.
> +  Context.NonAffineArrays.clear();
> +  Context.NonAffineAccesses.clear();

This should not be necessary. We allocate for each region a new 
detection context. If I remove this, all tests are still passing.

>      for (BasicBlock *BB : R.blocks())
>       for (BasicBlock::iterator I = BB->begin(), E = --BB->end(); I != E; ++I)
>         if (!isValidInstruction(*I, Context))
>           return false;
>
> +  if (hasNonAffineMemoryAccesses(Context))
> +    return false;
> +
>     return true;
>   }
>
> -- 1.7.6.4
>
>
> 0001-move-findArrayDimensions-to-ScalarEvolution.patch
>
>
>  From b1c13698acf130125be301e6e83780e9ec97c8b5 Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Thu, 8 May 2014 17:20:32 -0500
> Subject: [PATCH] move findArrayDimensions to ScalarEvolution
>
> we do not use the information from SCEVAddRecExpr to compute the shape of the array,
> so a better place for this function is in ScalarEvolution.

This one looks reasonable.

> 0001-fix-typo-in-debug-message.patch
>
>
>  From f2694b03ab90a20ae3aad4df48d6bf3abf067d51 Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Fri, 9 May 2014 11:09:03 -0500
> Subject: [PATCH] fix typo in debug message

This patch is trivial. No need to post it for review.

Cheers,
Tobias