[PATCH] D113101: [Polly][Isl] Use the function unsignedFromIslSize to manage a isl::size object. NFCI

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Nice one!

In D113101#3107069 <https://reviews.llvm.org/D113101#3107069>, @patacca wrote:

> I've tested it with the llvm test suite and it's showing 1 test failing but it's not related to this patch (`MultiSource/Applications/lambda-0.1.3/lambda.test`)

I remember I fixed the lambda failure. The buildbot shows green (http://meinersbur.de:8011/#/builders/76/builds/1306). Have you tested with the latest version of LLVM/Polly?



================
Comment at: polly/include/polly/Support/ISLTools.h:26
+#ifdef NDEBUG
+  // Don't force error checking in non-debug builds.
+  Size.is_error();
----------------
Suggestion for an explanation on why calling a getter disables error checking.


================
Comment at: polly/include/polly/Support/ISLTools.h:27
+  // Don't force error checking in non-debug builds.
+  Size.is_error();
+#else
----------------
Although `is_error` it doesn't have a `[[nodiscard]]` attribute (yet), adding `(void)` marks that we are intentionally ignoring the result.


================
Comment at: polly/include/polly/Support/ISLTools.h:523-528
+/// Check that @p End is valid and return an iterator from @p Begin to @p End
+///
+/// Use case example:
+///   for (unsigned i : rangeIslSize(0, Map.domain_tuple_dim()))
+///     // do stuff
+llvm::iota_range<unsigned> rangeIslSize(unsigned Begin, isl::size End);
----------------
nice!


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:201
   isl::map NextIterationMap = isl::map::universe(MapSpace);
-  for (auto u : seq<isl_size>(0, NextIterationMap.domain_tuple_dim().release()))
-    if (u != (isl_size)Dim)
+  for (auto u : rangeIslSize(0, NextIterationMap.domain_tuple_dim()))
+    if (u != Dim)
----------------
The coding standard rule is to only use `auto` if the type already appears on the right. Since you are removing `<isl_size>`, auto should be replaced by the concrete type.


================
Comment at: polly/lib/External/isl/include/isl/isl-noexceptions.h:201
 } // namespace isl
-
 #include <isl/id.h>
----------------
unrelated change?


================
Comment at: polly/lib/Transform/ScheduleTreeTransform.cpp:1188
   std::string IdentifierString(Identifier);
-  for (auto i : seq<isl_size>(0, Dims.release())) {
-    auto tileSize =
-        i < (isl_size)TileSizes.size() ? TileSizes[i] : DefaultTileSize;
+  for (auto i : rangeIslSize(0, Dims)) {
+    auto tileSize = i < TileSizes.size() ? TileSizes[i] : DefaultTileSize;
----------------



================
Comment at: polly/lib/Transform/ScheduleTreeTransform.cpp:1189
+  for (auto i : rangeIslSize(0, Dims)) {
+    auto tileSize = i < TileSizes.size() ? TileSizes[i] : DefaultTileSize;
     Sizes = Sizes.set_val(i, isl::val(Node.ctx(), tileSize));
----------------
[nit] Updating changed line to the latest coding standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113101