[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:2305
 
-  Set = Set.remove_divs();
+  isl::set &&simple_set = Set.remove_divs();
+  polly::simplify(simple_set);
----------------
Why an r-value reference?
"`simple_set`" does not adhere the LLVM [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | policy on naming local variables ]].
You could also just keep using `Set` as the code before.


================
Comment at: lib/Analysis/ScopInfo.cpp:2308
+
+  if (isl_set_n_basic_set(simple_set.get()) >= MaxDisjunctsInDomain)
+    simple_set = simple_set.simple_hull();
----------------
Consider using `RunTimeChecksMaxAccessDisjuncts` which the current trunk uses.


================
Comment at: lib/Analysis/ScopInfo.cpp:2351-2352
   // allocated array but we will never dereference it anyway.
-  assert(MaxPMA.dim(isl::dim::out) && "Assumed at least one output dimension");
+  assert(((!MaxPMA || !MinPMA) || MaxPMA.dim(isl::dim::out)) &&
+         "Assumed at least one output dimension");
+
----------------
You should decide between one of the following (not a mix of both):
1. Check for an error and return before reaching the assert
2. Let the assertion pass in case of `MaxPMA` being NULL.

Please tell me if you are unsure what to do.



================
Comment at: lib/Analysis/ScopInfo.cpp:2361-2362
 
+  if (!MinPMA || !MaxPMA)
+    return isl::stat::error;
+
----------------
Thanks.


Repository:
  rPLO Polly

https://reviews.llvm.org/D45066