[PATCH] D101486: [Dependence Analysis] Enable delinearization of fixed sized arrays

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour accepted this revision.
bmahjour added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3372
+        if (!isKnownNonNegative(S, Ptr))
+          FailedRangeCheck = true;
+        if (auto *SType = dyn_cast<IntegerType>(S->getType())) {
----------------
Meinersbur wrote:
> fhahn wrote:
> > artemrad wrote:
> > > fhahn wrote:
> > > > Can these now be early exits?
> > > I guess you are right. In my code we will exit early as soon as we do the next iteration of the loop (see loop condition.) With your proposal we skip a few instructions, for the cost of adding brackets to the if statement. I don't know if that is worth it, and whether it will make any impact on the performance as the loop calculation is fairly lightweight. 
> > > 
> > > I don't personally have a preference, if you insist I will add a return statement there.  
> > My thinking was that we could get rid of the `FailedRangeCheck` variable, and just `return false` here (and `return true;` in at the ned. Then have 
> > 
> > ```
> > if (!AllIndicesInRange() || !AllIndicesInRange())
> > ...
> > ```
> > 
> > It seems to me that this would simplify the code a bit and is more in line with the usual style in LLVM ( https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code)
> Some cleanups need to be done in the fail case:
> ```
> SrcSubscripts.clear();
> DstSubscripts.clear();
> ```
> This is why is suggested earlier to wrap the checks in another `Check` lambda. We already have a lambda, resulting in a lambda-in-a-lambda. Could be done (or refactored into its own function), but I am not sure whether the result will be any nicer.
> 
> Alternatively, one could wrap the clean-up in a destructor scope using a `llvm::make_scope_exit`.
Not sure, I follow your suggestion @Meinersbur . The lambda is just checking the range, returning true if in range and false otherwise. The cleanup is done on line 3383-3384.


================
Comment at: llvm/test/Analysis/DependenceAnalysis/PreliminaryNoValidityCheckFixedSize.ll:3
 ; RUN:   -da-disable-delinearization-checks | FileCheck %s
+; RUN: opt < %s -disable-output "-passes=print<da>" -aa-pipeline=basic-aa 2>&1 \
+; RUN:   | FileCheck --check-prefix=LIN %s
----------------
see comment below.


================
Comment at: llvm/test/Analysis/DependenceAnalysis/PreliminaryNoValidityCheckFixedSize.ll:30
 
+; LIN-LABEL: p2
+; LIN: da analyze - output [* * *]!
----------------
This is already being done in `llvm/test/Analysis/DependenceAnalysis/Preliminary.ll`. No need to repeat it here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101486/new/

https://reviews.llvm.org/D101486