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

[Johannes Doerfert via llvm-commits]

jdoerfert added a comment.

Hi Eli,

thanks for this patch! I can see the problem now and think your solution is the right way to fix it. Nevertheless I added quite a few (mostly minor) comments you might want to address or comment yourself.

Cheers,

  Johannes

PS. Can we expect you to work on Polly more regularly now?



> SCEVAffinator.h:117
> +  /// Whether to track the value of this expression precisely, rather than
> +  /// assuming it won't wrap.
> +  bool isPrecise(const llvm::SCEV *Expr);

I would rephrase this in a more direct way, e.g. True, if a possible wrap in expr is modeled, false otherwise.

> SCEVAffinator.cpp:120
> -  }
> -}
> -

I think the commit message should state explicitly that this caused ScalarEvoltuion to use flags for SCEVs that we introduced only as a means to an end. Also the solution should be mentioned more clearly: We avoid that by applying modulo semantics in the recursion now instead of afterwards.

> SCEVAffinator.cpp:250
> +      PWAC.first = addModuloSemantic(PWAC.first, Expr->getType());
>      PWAC = checkForWrapping(Expr, PWAC);
>    }

Did you omit the `else` here on purpose? If we add modulo semantics there is no wrapping (by construction)

> SCEVAffinator.cpp:253
>  
> -  if (!Factor->getType()->isIntegerTy(1))
> +  if (!Factor->isOne()) {
>      combine(PWAC, visitConstant(Factor), isl_pw_aff_mul);

1. This is unrelated.
2. Does isOne() evaluate on true on the value: "i1 -1" ?

> SCEVAffinator.cpp:262
>    PWAC.first = isl_pw_aff_coalesce(PWAC.first);
>    PWAC = checkForWrapping(Key.first, PWAC);
>  

Again this can be guarded by !isPrecise(..) can't it?

> SCEVAffinator.cpp:360
>    // bit-width is bigger than MaxZextSmallBitWidth we will employ overflow
>    // assumptions and assume the "former negative" piece will not exist.
>  

Comments are not adjusted.

> SCEVAffinator.cpp:372
>    if (OpCanWrap)
>      OpPWAC.first = addModuloSemantic(OpPWAC.first, Op->getType());
>  

This two lines looks like left over code but I am not 100% sure.

> 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"
> +

No triple pls.

> infeasible-rtc.ll:15
>  
> - at A = common global [128 x i32] zeroinitializer, align 16
> -
> -define void @test() nounwind uwtable {
> +define void @test(i64* %a) nounwind uwtable {
>  preheader:

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.

> 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

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 }

> zero_ext_of_truncate.ll:21
> +; CHECK-SAME: 256*floor((128 + tmp)/256) >= 257 + tmp - M) or 256*floor((128 + tmp)/256) <= tmp) };
>  ;
>  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

This looks like we could run into some regressions with this more agressive representation of wrapping behaviour. Did you observe problems?

Repository:
  rL LLVM

https://reviews.llvm.org/D25287