[PATCH] D45066: [Polly] [ScopInfo] Remove bail out condition in buildMinMaxAccess()

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Analysis/ScopInfo.cpp:2304
 
-  Set = Set.remove_divs();
-
+  Set = Set.simple_hull();
+  
----------------
cs15btech11044 wrote:
> On some analysis of this particular line, it turns out that on the application of simple hull, a bounded set results in its unbounded approximation, which is likely the cause of MinPMA and MaxPMA sets computed as null values. Almost all of the failed regression tests are indirectly caused by this unexpected behaviour.
Do you have an example for this behavior? If this happens too often or in too important cases, we may need to look for another solution. I would have hoped that this happens less often, or not at all, with affine_hull.


================
Comment at: lib/Analysis/ScopInfo.cpp:2336-2340
+  if(!MinPMA || !MaxPMA)
+    return isl::stat::error;
+
+  if (isl_ctx_last_error(Ctx.get()) == isl_error_invalid || isl_ctx_last_error(Ctx.get()) == isl_error_quota)
     return isl::stat::error;
----------------
These are redundant.


================
Comment at: lib/Analysis/ScopInfo.cpp:2359
   OneAff = OneAff.add_constant_si(1);
+
   LastDimAff = LastDimAff.add(OneAff);
----------------
[Nit] unnecessary change


================
Comment at: lib/Analysis/ScopInfo.cpp:2363
 
   MinMaxAccesses.push_back(std::make_pair(MinPMA, MaxPMA));
 
----------------
There should be a
```
if (!MinPMA || !MaxPMA)
  return isl::stat::error;
```
here. If any of the two is null, there will be another fail later, so it must never be added to `MinMaxAccesses`.

(And even `MaxPMA = MaxPMA.set_pw_aff(Pos, LastDimAff)` can fail, with the result of `nullptr` assigned to `MaxPMA`).


================
Comment at: lib/Analysis/ScopInfo.cpp:3199-3211
+    
+      if (isl_ctx_last_error(getIslCtx().get()) == isl_error_quota) {
+        invalidate(COMPLEXITY, DebugLoc());
+	return false;
+      }
+
       if (!Valid)
----------------
There are some inconsistencies in how things are handled here, true, but IMHO we can leave it alone for this patch (this is what I meant with "different issue" -- sorry for the confusion)


https://reviews.llvm.org/D45066