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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3372
+        if (!isKnownNonNegative(S, Ptr))
+          FailedRangeCheck = true;
+        if (auto *SType = dyn_cast<IntegerType>(S->getType())) {
----------------
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`.


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

https://reviews.llvm.org/D101486