[PATCH] D107225: [Polly][Isl] Move to the new-polly-generator branch version of isl-noexceptions.h. NFCI

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I don't see when you are using `isl::size::release()` and when `IslAssertNoError`. We talked about a follow-up patch for the assertions, but what is the concept for this patch?
Is it somehow possible to void the `static_cast<unsigned>` from `isl::size`? Part of the solution might be to make `islAssertNoError` return the unsigned result, as we talked about yesterday.

I tested the patch with the SPEC CPU benchmarks. We have one compilation failing which is `526.blender_r/src/blender/source/blender/editors/space_logic/logic_ops.c`. Depending on how clang is invoked, it either compiles forever/for too long or crashes with an isl-internal error. It does so even without this patch so it is unrelated. Surprisingly, it happens in a relatively simple call of `isl_set_gist_params` in `Scop::realignParams()` with the arguments:

  InvalidDomain = [p_0] -> {
    [] : p_0 >= 128 and -1 + p_0 <= 128*floor((64 + p_0)/128) <= p_0 and -255 + p_0 <= 32768*floor((16384 + p_0)/32768) <= p_0 and -65535 + p_0 <= 4194304*floor((2097152 + p_0)/4194304) <= p_0
  }
  Ctx = [p_0] -> {
     : -2147483648 <= p_0 <= 2147483647
  }

I suspect this snug in in an update of ISL. I will try to reproduce it and send a bug report to isl-development. Nothing to do for this patch.



================
Comment at: polly/include/polly/Support/ISLTools.h:26
+/// disable the mandatory state checking but do not enforce the error checking.
+inline void IslAssertNoError(const isl::size &Size) {
+#ifdef NDEBUG
----------------
"Lint: Pre-merge checks" is correct, we use camelCase for functions. Maybe you also have an idea for a better name than my suggestion.


================
Comment at: polly/include/polly/Support/ISLTools.h:535
+///     // do stuff
+llvm::iota_range<unsigned> rangeIslSize(unsigned Begin, isl::size End);
+
----------------
The asymmetry between the types of `Begin` and `End` bugs me. If `Begin` is always supposed to be 0, did you consider dropping the `Begin` parameter?


================
Comment at: polly/include/polly/Support/ISLTools.h:643
 /// @}
+
 } // namespace polly
----------------
[nit] unrelated whitespace change


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:1173-1174
   for (isl::set S : USet.get_set_list()) {
-    int Dim = S.tuple_dim();
+    int Dim = S.tuple_dim().release();
+    assert(Dim >= N);
     auto PMA = isl::pw_multi_aff::project_out_map(S.get_space(), isl::dim::set,
----------------
Use `IslAssertNoError`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107225/new/

https://reviews.llvm.org/D107225