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

Thanks for the update.

The `checked` property is annoying, especially for `isl::size`. For projects that use `assert` such as LLVM, the would be that under correct usage no error occurs and if it still does, it is a bug. That is, errors would only be checked explicitly in `LLVM_ENABLE_ASSERTIONS=ON` builds and otherwise assumed to not occur. the `checked` property forces us to always check for an error (or use `release()` as you did). Unfortunately, we cannot just use `assert(!Ndims.is_error())` since in non-Assert builds that code will be removed and `checked` no set to false.

Within an `IslMaxOperations` scope, `checked` is actually useful since we cannot exhaustively test when the limit is reached and the property would ensures that we consider a bailout control flow. `release()` also ignores that.

Did you consider instead of just calling `release()` everywhere, to introduce and "assert" that is not removed in release builds? Such as

  void IslAssertNoError(isl::size &S) {
  #ifdef NDEBUG
    // Don't force error checking in non-debug builds.
    S.is_error();
  #else
    // Assert on error in debug builds.
    assert(!S.is_error());
  #endif
  }

In functions that can can be called in an max-operations scope, we would instead use:

  if (S.is_error())
    return {}; // Or whatever represents the bailout control flow.

Before accepting, I want to run the patch on the llvm-test-suite where some of the max-operations conditions exceed.



================
Comment at: polly/lib/External/isl/include/isl/isl-noexceptions.h:202-216
+#include <isl/id.h>
+#include <isl/space.h>
+#include <isl/val.h>
+#include <isl/aff.h>
+#include <isl/set.h>
+#include <isl/map.h>
+#include <isl/ilp.h>
----------------
[not a change request] Includes in the middle of the file are usually discouraged as it makes its content dependent on where it is included and things like precompiled headers and module impossible.

cpp-checked.h does this for some reason, which means at most we suggest upstream to change that.


================
Comment at: polly/lib/Support/GICHelper.cpp:240-245
-  isl::union_pw_multi_aff().dump();
-  isl::union_pw_multi_aff_list().dump();
-  isl::union_set().dump();
-  isl::union_set_list().dump();
-  isl::val().dump();
-  isl::val_list().dump();
----------------
Not all the classes are exported/needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107225