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

[Riccardo Mori via Phabricator via llvm-commits]

patacca added a comment.

In D107225#2939965 <https://reviews.llvm.org/D107225#2939965>, @Meinersbur wrote:

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

My idea was to rename and maybe reimplement the `IslAssertNoError` in a different patch because this one is getting pretty big and has already a lot of changes.
I know that in the last meeting we agreed to include both `IslAssertNoError` and `isl::size::release` in this patch but I am now thinking of landing the patch withouth the `IslAssertNoError` and opening a new one in which the assertion and the casting to `unsigned` is better handled. Is it ok?

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

I've run the llvm-test-suite and I also got a single failing test case: `MultiSource/Applications/lambda-0.1.3/lambda.test`
I am still unsure if it is related to this patch or not but I am investigating



================
Comment at: polly/include/polly/Support/ISLTools.h:535
+///     // do stuff
+llvm::iota_range<unsigned> rangeIslSize(unsigned Begin, isl::size End);
+
----------------
Meinersbur wrote:
> 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?
I've put the `Begin` parameter just for convenience: sometimes (ScopInfo.cpp:681 for example) we start the iteration from a different value. I can remove it if you think it's better


================
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,
----------------
Meinersbur wrote:
> Use `IslAssertNoError`?
I thought that since this one is a stronger assertion I could just remove the `IslAssertNoError`.
We aren't getting the call to `isl::size::is_error` though so it might be better to add it anyway


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107225