[polly] [ScopInfo] Don't use isl_val_get_num_si.

[efriedma]

Summary

isl_val_get_num_si crashes on overflow, so don't use it on arbitrary integers. Instead, convert to an APInt and explicitly check for overflow.

Testcase only crashes on platforms where long is 32 bits because of the signature of isl_val_get_num_si; not sure if it's possible to write a testcase which crashes if long is 64 bits.

There are a few other places in polly which use isl_val_get_num_si; they probably need to be fixed as well. I don't think polly uses any of the other "long" isl APIs in an unsafe manner.

[Meinersbur]

lib/Analysis/ScopInfo.cpp

3429 ↗

(On Diff #130022)

Why not making ValInt (and the Int vector etc) a long?

[Meinersbur]

lib/Analysis/ScopInfo.cpp

3431 ↗

(On Diff #130022)

There might be a misconception here: isl_val_is_int only returns false for fractions. We don't use rational polyhedral sets, so the branch is always taken, even if Val exceeds any range.

3439 ↗

(On Diff #130022)

Does it make sense to fall-through with ValInt = 1 if the value exceeds the range? It seems to be intended, but I don't see what the effect is.

[efriedma]

lib/Analysis/ScopInfo.cpp

3429 ↗

(On Diff #130022)

I don't see how making ValInt a 64-bit integer is any better than making it a 32-bit integer. At least, it's orthogonal to the crash fix.

3431 ↗

(On Diff #130022)

It looks like isl_aff_get_denominator_val always returns either a nan or an int. No idea if nan can actually show up here.

3439 ↗

(On Diff #130022)

The effect is that it eventually fails the transform, because we set CanFold to false.

[Meinersbur]

LGTM.

Fallback value can be discussed separately, it also exists in the current codebase.

lib/Analysis/ScopInfo.cpp

3429 ↗

(On Diff #130022)

OK, from your post on isl-dev, I was under the impression that this was a platform issue.

3432–3434 ↗

(On Diff #130022)

One could check whether it fits an int more directly (without requiring APInt) using isl_val_cmp_si (twice: for INT_MIN and INT_MAX) or isl_val_n_abs_num_chunks(Val) < 32.

3439 ↗

(On Diff #130022)

Only Int[0] is checked on line 3473, but the fallback can happen on any dimension. Am I missing something?

[efriedma]

lib/Analysis/ScopInfo.cpp

3429 ↗

(On Diff #130022)

The crash happens inside isl_val_get_num_si (it has an explicit call to "die()" if the value doesn't fit into a "long").

Thinking about it a bit more, I guess the implicit truncation in the expression "ValInt = isl_val_get_num_si(Val);" could cause polly to miscompile a loop? I'm not sure how to construct a testcase, though.

3439 ↗

(On Diff #130022)

We check the other dimensions in the loop immediately after that.

polly/trunk/lib/Analysis/ScopInfo.cpp

for (int i = 0; i < Dims; i++) {
 
       if (isl_basic_set_dim(DimHull, isl_dim_div) == 1) {
         isl_aff *Diff = isl_basic_set_get_div(DimHull, 0);
         isl_val *Val = isl_aff_get_denominator_val(Diff);
         isl_aff_free(Diff);
 
         int ValInt = 1;
 
-        if (isl_val_is_int(Val))
-          ValInt = isl_val_get_num_si(Val);
+        if (isl_val_is_int(Val)) {
+          auto ValAPInt = APIntFromVal(Val);
+          if (ValAPInt.isSignedIntN(32))
+            ValInt = ValAPInt.getSExtValue();
+        } else {
           isl_val_free(Val);
+        }
 
         Int.push_back(ValInt);
 
         isl_constraint *C = isl_constraint_alloc_equality(
             isl_local_space_from_space(isl_map_get_space(Transform)));
         C = isl_constraint_set_coefficient_si(C, isl_dim_out, i, ValInt);
         C = isl_constraint_set_coefficient_si(C, isl_dim_in, i, -1);
         Transform = isl_map_add_constraint(Transform, C);

polly/trunk/test/ScopInfo/multidim_fold_constant_dim.ll

 ; CHECK-NEXT:             [n] -> { Stmt_for_body3[i0, i1] -> MemRef_A[i0, 2i1] };
 ; CHECK-NEXT:         MustWriteAccess :=  [Reduction Type: NONE] [Scalar: 0]
 ; CHECK-NEXT:             [n] -> { Stmt_for_body3[i0, i1] -> MemRef_A[i0, 2i1] };
 
 source_filename = "/tmp/test.c"
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 %struct.com = type { double, double }
+%struct.com2 = type { [20000000000 x double] }
 
 define void @foo(i64 %n, %struct.com* %A) {
 entry:
   br label %for.cond
 
 for.cond:                                         ; preds = %for.inc7, %entry
   %i.0 = phi i64 [ 0, %entry ], [ %inc8, %for.inc7 ]
   %exitcond1 = icmp ne i64 %i.0, 100

 for.inc7:                                         ; preds = %for.end
   %inc8 = add nuw nsw i64 %i.0, 1
   br label %for.cond
 
 for.end9:                                         ; preds = %for.cond
   ret void
 }
 
-define i32 @main() {
+; CHECK:      Arrays {
+; CHECK-NEXT:     double MemRef_O[*][%n]; // Element size 8
+; CHECK-NEXT: }
+
+define void @foo_overflow(i64 %n, %struct.com2* nocapture %O) local_unnamed_addr #0 {
 entry:
-  %A = alloca [100 x [1000 x %struct.com]], align 16
-  %tmp = getelementptr inbounds [100 x [1000 x %struct.com]], [100 x [1000 x %struct.com]]* %A, i64 0, i64 0, i64 0
-  call void @foo(i64 1000, %struct.com* nonnull %tmp)
-  ret i32 0
+  br label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.cond.cleanup3
+  ret void
+
+for.body:                                         ; preds = %for.cond.cleanup3, %entry
+  %i.024 = phi i64 [ 0, %entry ], [ %inc12, %for.cond.cleanup3 ]
+  %0 = mul nsw i64 %i.024, %n
+  %arrayidx = getelementptr inbounds %struct.com2, %struct.com2* %O, i64 %0
+  br label %for.body4
+
+for.cond.cleanup3:                                ; preds = %for.body4
+  %inc12 = add nuw nsw i64 %i.024, 1
+  %exitcond25 = icmp eq i64 %inc12, 100
+  br i1 %exitcond25, label %for.cond.cleanup, label %for.body
+
+for.body4:                                        ; preds = %for.body4, %for.body
+  %j.023 = phi i64 [ 0, %for.body ], [ %inc, %for.body4 ]
+  %arrayidx5 = getelementptr inbounds %struct.com2, %struct.com2* %arrayidx, i64 %j.023
+  %Real = getelementptr inbounds %struct.com2, %struct.com2* %arrayidx5, i64 0, i32 0
+  %arrayidx6 = getelementptr inbounds [20000000000 x double], [20000000000 x double]* %Real, i64 0, i64 1
+  %1 = load double, double* %arrayidx6, align 8
+  %arrayidx10 = getelementptr inbounds [20000000000 x double], [20000000000 x double]* %Real, i64 0, i64 0
+  %2 = load double, double* %arrayidx10, align 8
+  %add = fadd double %1, %2
+  store double %add, double* %arrayidx10, align 8
+  %inc = add nuw nsw i64 %j.023, 1
+  %exitcond = icmp eq i64 %inc, 1000
+  br i1 %exitcond, label %for.cond.cleanup3, label %for.body4
 }