[PATCH] D25287: [SCEVAffinator] Make precise modular math more correct.

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

Looks almost good. I am not sure about the i1 thing and this one test case though. Does lnt pass?



================
Comment at: lib/Support/SCEVAffinator.cpp:215
+  // We assume nsw expressions never overflow.
+  if (auto *NAry = dyn_cast<SCEVNAryExpr>(Expr))
+    if (NAry->getNoWrapFlags() & SCEV::FlagNSW)
----------------
We have a helper function here

```
getNoWrapFlags(Expr)
```
but I do not care too much.


================
Comment at: lib/Support/SCEVAffinator.cpp:253
 
-  if (!Factor->getType()->isIntegerTy(1))
-    combine(PWAC, visitConstant(Factor), isl_pw_aff_mul);
+  combine(PWAC, visitConstant(Factor), isl_pw_aff_mul);
+  if (computeModuloForExpr(Key.first))
----------------
The fact that you can remove the guard and the test still work is merely a coincidence and should not be commited this way. As soon as (for whatever reason) computeModuloForExpr(..) does not return true for an i1 expression we will have a hard to find bug here.


================
Comment at: test/ScopInfo/bool-addrec.ll:12
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n8:16:32-S64"
+target triple = "armv4t--linux-gnueabi"
+
----------------
jdoerfert wrote:
> No triple pls.
No triple pls.


================
Comment at: test/ScopInfo/integers.ll:115
 ; CHECK:  'bb => return' in function 'f6'
-; CHECK: [n] -> { Stmt_bb[0] : n = 3 };
+; CHECK: [n] -> { Stmt_bb[i0] : i0 >= 0 and 8*floor((5 + n)/8) <= 5 + n - i0 };
   %exitcond = icmp eq i3 %indvar, %sub
----------------
efriedma wrote:
> jdoerfert wrote:
> > It his hard to verify this domain without the {assumed_,invalid_,}context of the scop. Could you add those to the test or at least the review?
> > 
> > Currently all I know that the domain shown above doesn't make sense as it contains the following points (note the nsw for %indvars and the i0 > 3):
> > 
> > ```
> > 
> > [n] -> { Stmt_bb[i0] : n = -1 and i0 = 3 }
> > [n] -> { Stmt_bb[i0] : n = -1 and i0 = 1 }
> > [n] -> { Stmt_bb[i0] : n = -1 and i0 = 4 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 5 }
> > [n] -> { Stmt_bb[i0] : n = 1 and i0 = 6 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 7 }
> > [n] -> { Stmt_bb[i0] : n = 3 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = 1 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = 0 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = -1 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = -2 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = -3 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = -4 and i0 = 0 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 6 }
> > [n] -> { Stmt_bb[i0] : n = 0 and i0 = 5 }
> > [n] -> { Stmt_bb[i0] : n = 1 and i0 = 5 }
> > [n] -> { Stmt_bb[i0] : n = 1 and i0 = 4 }
> > [n] -> { Stmt_bb[i0] : n = 0 and i0 = 4 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 4 }
> > [n] -> { Stmt_bb[i0] : n = -2 and i0 = 3 }
> > [n] -> { Stmt_bb[i0] : n = 1 and i0 = 3 }
> > [n] -> { Stmt_bb[i0] : n = 0 and i0 = 3 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 3 }
> > [n] -> { Stmt_bb[i0] : n = -1 and i0 = 2 }
> > [n] -> { Stmt_bb[i0] : n = -3 and i0 = 2 }
> > [n] -> { Stmt_bb[i0] : n = -2 and i0 = 2 }
> > [n] -> { Stmt_bb[i0] : n = 1 and i0 = 2 }
> > [n] -> { Stmt_bb[i0] : n = 0 and i0 = 2 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 2 }
> > [n] -> { Stmt_bb[i0] : n = -4 and i0 = 1 }
> > [n] -> { Stmt_bb[i0] : n = -3 and i0 = 1 }
> > [n] -> { Stmt_bb[i0] : n = -2 and i0 = 1 }
> > [n] -> { Stmt_bb[i0] : n = 1 and i0 = 1 }
> > [n] -> { Stmt_bb[i0] : n = 0 and i0 = 1 }
> > [n] -> { Stmt_bb[i0] : n = 2 and i0 = 1 }
> > 
> > ```
> Full output follows.  I think the domain is right if you ignore the nsw... but you're right, my patch isn't correctly honoring the nsw flag for very narrow addition operations.  I'll fix that.
> 
> ```
> Printing analysis 'Polly - Create polyhedral description of Scops' for region: 'bb => return' in function 'f6':
>     Function: f6
>     Region: %bb---%return
>     Max Loop Depth:  1
>     Invariant Accesses: {
>     }
>     Context:
>     [n] -> {  : -4 <= n <= 3 }
>     Assumed Context:
>     [n] -> {  :  }
>     Invalid Context:
>     [n] -> {  : 1 = 0 }
>     p0: %n
>     Arrays {
>         i3 MemRef_a[*]; // Element size 1
>     }
>     Arrays (Bounds as pw_affs) {
>         i3 MemRef_a[*]; // Element size 1
>     }
>     Alias Groups (0):
>         n/a
>     Statements {
>         Stmt_bb
>             Domain :=
>                 [n] -> { Stmt_bb[i0] : i0 >= 0 and 8*floor((5 + n)/8) <= 5 + n - i0 };
>             Schedule :=
>                 [n] -> { Stmt_bb[i0] -> [i0] };
>             MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 0]
>                 [n] -> { Stmt_bb[i0] -> MemRef_a[i0] };
>     }
> ```
The domain in the test is different from the one you posted, isn't it? I'm confused, sry.


================
Comment at: test/ScopInfo/truncate-1.ll:15
 ; CHECK:       Domain :=
-; CHECK-NEXT:    [N] -> { Stmt_for_body[i0] : i0 >= 0 and 256*floor((128 + N)/256) < N - i0 };
+; CHECK-NEXT:    [N] -> { Stmt_for_body[i0] : 0 <= i0 < N };
 ;
----------------
For now OK I guess.


================
Comment at: test/ScopInfo/zero_ext_of_truncate.ll:12
 ;
+; FIXME: The truncated value should be a paramter.
 ; CHECK:         Assumed Context:
----------------
True but that is not that easy to decide. If there was a second use of "%tmp" the situation might look different. Anyway, if you want to work on this we could see if we can come up with a scheme.


Repository:
  rL LLVM

https://reviews.llvm.org/D25287