[PATCH] D42129: [polly] [ScopInfo] Don't use isl_val_get_num_si.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Analysis/ScopInfo.cpp:3429
 
         int ValInt = 1;
 
----------------
Meinersbur wrote:
> Why not making `ValInt` (and the `Int` vector etc) a long?
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.


================
Comment at: lib/Analysis/ScopInfo.cpp:3431
 
-        if (isl_val_is_int(Val))
-          ValInt = isl_val_get_num_si(Val);
----------------
Meinersbur wrote:
> 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.
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.


================
Comment at: lib/Analysis/ScopInfo.cpp:3439
 
         Int.push_back(ValInt);
 
----------------
Meinersbur wrote:
> 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.
The effect is that it eventually fails the transform, because we set CanFold to false.


Repository:
  rPLO Polly

https://reviews.llvm.org/D42129