[PATCH] D11725: [DependenceAnalysis] Ensure All Recurrences are Affine

[Matthew Simpson via llvm-commits]

mssimpso added a comment.

Sanjoy,

Thanks very much for joining the review and providing feedback. I very much appreciate it!

> What is the difference between `DependenceAnalysis::tryDelinearize` and `ScalarEvolution::delinearize`?


As far as I know, there shouldn't be a difference here regarding the actual computation. In fact, before the most recent rewrite that split the computation into 3 phases, tryDelinearize called delinearize directly. Perhaps @sebpop can provide more insight. Otherwise, I will look into restoring that feature to avoid duplication.

> It looks like `SCEVDivision` bails out on things other than non-affine add recs (e.g. on `SCEVUnknown`s).  Why are those okay? IOW, is the failing assert too strong?


If you're asking why the divide function gives up on some cases (appropriately setting the quotient to be zero and the remainder to be the original dividend) and instead asserts on the non-affine add rec case, I think that's a valid point. Does it make sense to quietly give up here instead of asserting?

> This is assuming that the only problem with a nested non-affine add rec is that the `SCEVDivision` aborts, are there other problems too?


None that I'm aware of specifically, although see @ssijaric's comment above regarding a similar issue in Polly. The add rec does indeed need to be affine to divide. If it is not, getSetRecurrence() will construct a new add rec, and so on, and we may never be able to traverse the entire expression. It also seems that having an easy way to recursively check whether an add rec is affine would be useful. Any more thoughts you might have on bailing vs. asserting would be helpful. Thanks, again.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:320
@@ -319,1 +319,3 @@
 
+    /// isAffineRecursive - Return true if all SCEVAddRexExprs contained in
+    /// this expression are affine.
----------------
sanjoy wrote:
> Nit: `SCEVAddRecExprs`
Nice catch! Thank you.

================
Comment at: lib/Analysis/DependenceAnalysis.cpp:3281
@@ -3280,2 +3280,3 @@
   const SCEVAddRecExpr *DstAR = dyn_cast<SCEVAddRecExpr>(DstSCEV);
-  if (!SrcAR || !DstAR || !SrcAR->isAffine() || !DstAR->isAffine())
+  if (!SrcAR || !DstAR || !SrcAR->isAffineRecursive() ||
+      !DstAR->isAffineRecursive())
----------------
sanjoy wrote:
> Why not move this check to `ScalarEvolution::computeAccessFunctions`?
I agree. computeAccessFunctions already checks that the expression is an affine add rec. We would only need to change the check to the recursive version.

================
Comment at: test/Analysis/DependenceAnalysis/AffineSCEVAddRecExpr.ll:4
@@ +3,3 @@
+;
+; This is a reduced test case from a benchmark in spec2006. We check that this
+; code does not abort.
----------------
mcrosier wrote:
> Is it valuable to comments this is a reduced test case from spec?  Perhaps we could just say:
> 
> "We check that this reduced test case does not abort."
> 
> This suggestion doesn't require a revised patch, if accepted.  Just make the change before committing the patch.
I agree. Thanks, Chad.


http://reviews.llvm.org/D11725