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

[Johannes Doerfert via llvm-commits]

On 10/06, Eli Friedman wrote:
> Yes, you can expect more polly patches from me in the near future. :)
Cool.

> Quick summary of what's going on with SCEV flags: integer math in LLVM IR is modular.  Integer math in isl is arbitrary-precision.  Modeling LLVM IR math correctly in isl requires either adding assumptions that math doesn't actually overflow, or explicitly wrapping the math.  However, expressions with the "nsw" flag are special; we can pretend they're arbitrary-precision because it's undefined behavior if the result wraps.  SCEV expressions based on IR instructions with an nsw flag also carry an nsw flag (roughly; actually, the real rule is a bit more complicated, but the details don't matter here).
> 
> Before this patch, SCEV flags were also overloaded with an additional function: the ZExt code was mutating SCEV expressions as a hack to indicate to checkForWrapping that we don't need to add assumptions to the operand of a ZExt; it'll add explicit wrapping itself.  This kind of works... the problem is that if anything else ever touches that SCEV expression, it'll get confused by the incorrect flags.
> 
> Instead, with this patch, we make the decision about whether to explicitly wrap the math a bit earlier, basing the decision purely on the SCEV expression itself, and not its users.
> 
> (I'll adapt this description for the commit message.)
I guess the above text was for the commit message, right?


> > jdoerfert wrote in SCEVAffinator.cpp:250
> > Did you omit the `else` here on purpose? If we add modulo semantics there is no wrapping (by construction)
> 
> I can.  checkForWrapping is essentially a no-op on the result of addModuloSemantic, since the value can't actually be out of range.
That is my point.

> > jdoerfert wrote in SCEVAffinator.cpp:253
> > 1. This is unrelated.
> > 2. Does isOne() evaluate on true on the value: "i1 -1" ?
> 
> Okay, I can separate it out.
> 
> Yes, isOne() is true for i1 -1.
OK, that is the only case we need the check to work for.

> > jdoerfert wrote in SCEVAffinator.cpp:372
> > This two lines looks like left over code but I am not 100% sure.
> 
> Yes, I think so too; I'll remove them.
Good.

> > jdoerfert wrote in infeasible-rtc.ll:15
> > Why did you modify the originial? Wasn't it refused anymore? Could we have both tests? The new one and the old one with a different comment + check line so people reading the patch later have more information.
> 
> I modified it because the old test wasn't getting rejected: with this patch, we use modular math and correctly compute the trip count.
> 
> Sure, I can keep around the old test, too, if you think it's appropriate.
I do not have strong feelings about it but I guess it might help to
understand the patch later.

> > jdoerfert wrote in integers.ll:115
> > 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.
OK, I'll wait for a new revision then.

>   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] };
>       }
> 
> > jdoerfert wrote in zero_ext_of_truncate.ll:21
> > This looks like we could run into some regressions with this more agressive representation of wrapping behaviour. Did you observe problems?
> 
> We have a related patch to avoid modeling truncates for loop-invariant expressions, which makes the complexity of truncate expressions less problematic (with or without this patch).  Hopefully we'll get something posted soon.
> 
> Beyond that, I probably should have tested a little more carefully before I posted this patch; this version is more aggressive than what we've been using, and it causes a problem on one of our internal tests.  I might tweak the heuristic.
It looks that way for the last test and I can see this causing the
"complexity heuristic" to trigger more often too. Maybe we can use the
complexity check as an additional source to decide if modeling the wrap
or assuming no-wrap is the better option.

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland Informatics Campus, Germany
Building E1.3, Room 4.31

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161007/951b5a70/attachment.sig>