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

[Riccardo Mori via Phabricator]

patacca created this revision.
Herald added subscribers: ormris, bmahjour, steven_wu, kbarton, hiraditya, nemanjai.
Herald added a reviewer: bollu.
patacca updated this revision to Diff 384510.
patacca added a comment.
patacca retitled this revision from "[Polly][Isl] Introduce unsignedFromIslSize and islAssert. NFCI" to "[Polly][Isl] Use the function unsignedFromIslSize to manage a isl::size object. NFCI".
patacca edited the summary of this revision.
patacca added a reviewer: Meinersbur.
patacca added a project: Polly.
patacca added a subscriber: pollydev.
patacca edited the summary of this revision.
patacca published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Fix code style


This is part of an effort to reduce the differences between the custom C++ bindings used right now by polly in lib/External/isl/include/isl/isl-noxceptions.h and the official isl C++ interface.
In the official interface the type `isl::size` cannot be casted to an unsigned without previously having checked if it contains a valid value with the function `isl::size::is_error()`.
For this reason two helping functions have been added:

- `IslAssert`: assert that no errors are present in debug builds and just disables the mandatory error check in non-debug builds
- `unisgnedFromIslSIze`: cast the `isl::size` object to `unsigned`

Changes made:

- Add the functions `IslAssert` and `unsignedFromIslSize`
- Add the utility function `rangeIslSize()`
- Retype `MaxDisjunctsInDomain` from `int` to `unsigned`
- Retype `RunTimeChecksMaxAccessDisjuncts` from `int` to `unsigned`
- Retype `MaxDimensionsInAccessRange` from `int` to `unsigned`
- Replaced some usages of `isl_size` to `unsigned` since we aim not to use `isl_size` anymore
- `isl-noexceptions.h` has been generated by https://github.com/patacca/isl/commit/e704f73c88f0b4d88e62e447bdb732cf5914094b

No functional change intended.


Repository:
rG LLVM Github Monorepo

https://reviews.llvm.org/D113101

Files:
polly/include/polly/ScheduleTreeTransform.h
polly/include/polly/ScopInfo.h
polly/include/polly/Support/ISLTools.h
polly/lib/Analysis/DependenceInfo.cpp
polly/lib/Analysis/ScopBuilder.cpp
polly/lib/Analysis/ScopInfo.cpp
polly/lib/CodeGen/BlockGenerators.cpp
polly/lib/CodeGen/PPCGCodeGeneration.cpp
polly/lib/Exchange/JSONExporter.cpp
polly/lib/External/isl/include/isl/isl-noexceptions.h
polly/lib/Support/ISLTools.cpp
polly/lib/Transform/FlattenAlgo.cpp
polly/lib/Transform/MatmulOptimizer.cpp
polly/lib/Transform/MaximalStaticExpansion.cpp
polly/lib/Transform/ScheduleOptimizer.cpp
polly/lib/Transform/ScheduleTreeTransform.cpp
polly/lib/Transform/Simplify.cpp
polly/lib/Transform/ZoneAlgo.cpp
polly/unittests/Isl/IslTest.cpp

[Riccardo Mori via Phabricator]

patacca added a comment.

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 added the `IslAssert` function with the idea that it might be useful but in the end we just use `unsignedFromIslSize` so tell me if you think it's better to remove it

Repository:
rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113101

[Michael Kruse via Phabricator]

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.

[Riccardo Mori via Phabricator]

patacca added a comment.

In D113101#3107313 <https://reviews.llvm.org/D113101#3107313>, @Meinersbur wrote:

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

For some reason I cannot connect to the link you posted. Anyway after I tested it again with a fresh new build of llvm-suite the error in the lambda test disappeared.

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

Meinersbur wrote:
> unrelated change?
This one was a leftover blank space that was removed in this commit https://github.com/patacca/isl/commit/e704f73c88f0b4d88e62e447bdb732cf5914094b
Since then nobody actually updated `isl-noexceptions.h`

[Riccardo Mori via Phabricator]

patacca updated this revision to Diff 384741.
patacca added a comment.

- Fix coding style (replace `auto` with `unsigned`)
- Better explainatory comment for `islAssert`
- Mark the result of intentionally discarded value with `void` casting

Repository:
rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113101

[Michael Kruse via Phabricator]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for picking this up again.

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

patacca wrote:
> Meinersbur wrote:
> > unrelated change?
> This one was a leftover blank space that was removed in this commit https://github.com/patacca/isl/commit/e704f73c88f0b4d88e62e447bdb732cf5914094b
> Since then nobody actually updated `isl-noexceptions.h`

Seems that was me. Different editors have different understanding of whether `\n` is a line terminator (i.e. even the last line ends with `\n`) or line separator (i.e. no `\n` on last line). See https://en.wikipedia.org/wiki/Newline#Interpretation

[Riccardo Mori via Phabricator]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG44596fe6a95e: [Polly][Isl] Use the function unsignedFromIslSize to manage a isl::size object. (authored by patacca).

Repository:
rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113101

[Jay Foad via Phabricator]

foad added a comment.

Hi, this seems to have introduced a couple of warnings in my LLVM build (a release build with polly enabled, using clang 10 as the host compiler):

[3071/3609] Building CXX object tools/polly/lib/CMakeFiles/obj.Polly.dir/Support/ISLTools.cpp.o
/home/jayfoad2/git/llvm-project/polly/lib/Support/ISLTools.cpp:220:14: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
assert(Pos < NumDims && "Dimension index must be in range");
~~~ ^ ~~~~~~~
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
(static_cast <bool> (expr) \
^~~~
/home/jayfoad2/git/llvm-project/polly/lib/Support/ISLTools.cpp:241:14: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
assert(Pos < NumDims && "Dimension index must be in range");
~~~ ^ ~~~~~~~
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
(static_cast <bool> (expr) \
^~~~
2 warnings generated.