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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM.

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



================
Comment at: lib/Analysis/ScopInfo.cpp:3429
 
         int ValInt = 1;
 
----------------
efriedma wrote:
> 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.
OK, from your post on isl-dev, I was under the impression that this was a platform issue.


================
Comment at: lib/Analysis/ScopInfo.cpp:3432-3434
+          auto ValAPInt = APIntFromVal(Val);
+          if (ValAPInt.isSignedIntN(32))
+            ValInt = ValAPInt.getSExtValue();
----------------
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`.


================
Comment at: lib/Analysis/ScopInfo.cpp:3439
 
         Int.push_back(ValInt);
 
----------------
efriedma wrote:
> 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.
Only `Int[0]` is checked on line 3473, but the fallback can happen on any dimension. Am I missing something?


Repository:
  rPLO Polly

https://reviews.llvm.org/D42129