[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:
> 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.
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.


================
Comment at: lib/Analysis/ScopInfo.cpp:3439
 
         Int.push_back(ValInt);
 
----------------
Meinersbur wrote:
> 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?
We check the other dimensions in the loop immediately after that.


Repository:
  rPLO Polly

https://reviews.llvm.org/D42129