Page MenuHomePhabricator

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

Authored by patacca on Jul 31 2021, 9:24 AM.

Details

Summary

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.

With this commit we are moving from the polly-generator branch to the new-polly-generator branch that is more mantainable and is based on the official C++ interface cpp-checked.h.

Changes made:

  • There are now many sublcasses for isl::ast_node representing different isl types. Use isl::ast_node_for, isl::ast_node_user, isl::ast_node_block and isl::ast_node_mark where needed.
  • There are now many sublcasses for isl::schedule_node representing different isl types. Use isl::schedule_node_mark, isl::schedule_node_extension, isl::schedule_node_band and isl::schedule_node_filter where needed.
  • Replace the isl::*::dump with dumpIslObj since the isl dump method is not exposed in the C++ interface.
  • isl::schedule_node::get_child has been renamed to isl::schedule_node::child
  • isl::pw_multi_aff::get_pw_aff has been renamed to isl::pw_multi_aff::at
  • The constructor isl::union_map(isl::union_pw_multi_aff) has been replaced with the static method isl::union_map::from()
  • Replace usages of isl::val::add_ui with isl::val::add
  • isl::union_set_list::alloc is now a constructor
  • All the isl_size values are now wrapped inside the class isl::size use isl::size::release to get the internal isl_size value where needed.
  • isl-noexceptions.h has been generated by https://github.com/patacca/isl/commit/73f5ed1f4d1f72582f731590ef9e43d9ab1956ad

No functional change intended.

Diff Detail

Event Timeline

patacca created this revision.Jul 31 2021, 9:24 AM
patacca retitled this revision from [Polly][Isl] Use the new-polly-generator branch version of isl-noexceptions.h. NFCI to [Polly][Isl] Move to the new-polly-generator branch version of isl-noexceptions.h. NFCI.Jul 31 2021, 9:55 AM
patacca edited the summary of this revision. (Show Details)
patacca added a reviewer: Meinersbur.
patacca added a project: Restricted Project.
patacca edited the summary of this revision. (Show Details)
patacca updated this revision to Diff 363307.Jul 31 2021, 9:59 AM

Revert unrelated change

patacca published this revision for review.Jul 31 2021, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2021, 10:00 AM
patacca edited the summary of this revision. (Show Details)Jul 31 2021, 10:00 AM
patacca updated this revision to Diff 363452.Aug 2 2021, 5:26 AM

Add error checks for methods with callbacks in isl-noexceptions.h
This is to remove the warnings during compilation

patacca edited the summary of this revision. (Show Details)Aug 2 2021, 5:27 AM

Thanks for all the effort. Unfortunately the boolean class had a reason to exist. For instance, with isl_set_empty there are 3 possible outcomes:

  • true if the set is empty
  • false if the set contains at least one element
  • error if the result could not be computed, for instance because the argument is NULL or the max_operations counter has been exceeded.

In cpp.h, with exceptions disabled, the 3rd case aborts the program. The argument being NULL could be tested before calling the method, but there is no means to determine whether the max_operations counter is going to be exceeded in the next call. We don't want to crash but to execute some bail-out code. I expected some tests to fail.
cpp-checked.h still has a boolean class and I think there is no way around using it instead of cpp.h. In contrast to isl-noexceptions.h's boolean, the checked on requires the boolean state to be checked for errors to avoid that an error state is accidentally interpreted that same way as true if used like this:

isl::boolean = isl_bool_error;
if (b) {
  ...
}

I might have given the impression that cpp.h is what we need; at that time I though cpp.h was just boolean::checked, but it is missing the entire boolean class.

polly/include/polly/Support/GICHelper.h
189

With isl being a C API and the official C++ binding not having a dump() method, there is nothing " dump() method" could refer to.

patacca updated this revision to Diff 364818.Aug 6 2021, 9:20 AM
patacca updated this revision to Diff 364819.Aug 6 2021, 9:22 AM

Fix comment for ISL_DUMP_OBJECT

bmahjour removed a subscriber: bmahjour.Aug 6 2021, 9:23 AM
patacca updated this revision to Diff 364823.Aug 6 2021, 9:28 AM

Fix some typo

patacca edited the summary of this revision. (Show Details)Aug 6 2021, 9:33 AM

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.

polly/lib/External/isl/include/isl/isl-noexceptions.h
202–217

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

polly/lib/Support/GICHelper.cpp
240–245

Not all the classes are exported/needed?

patacca added inline comments.Aug 8 2021, 8:45 AM
polly/lib/Support/GICHelper.cpp
240–245

Some classes have been removed from the interface and were not used anywhere in polly. These are ast_expr_list, constraint_list, pw_qpolynomial, qpolynomial, union_map_list and union_pw_multi_aff_list

patacca updated this revision to Diff 365038.Aug 8 2021, 10:41 AM

Rebase to main

Merge commit 50eaa82cdbc72588f0841658bbad9695855eba85 "[Polly][test] Test difference between isl::stat:ok() and isl::stat::error()." and 0f50ffb3365eefaf114f3d1de6e8acee4fe8f169 "[Polly][test] Add tests for IslMaxOperationsGuard."

patacca updated this revision to Diff 365462.Aug 10 2021, 6:49 AM

Add IslAssertNoError and replace some usage of isl::size::release
Add the utility function rangeIslSize
Retype MaxDisjunctsInDomain from int to unsigned
Retype RunTimeChecksMaxAccessDisjuncts from int to unsigned
Add the operators to check the equality and the inequality of two isl::size
The function getNumScatterDims now returns an unsigned instead of isl_size

This is a first patch to use IslAssertNoError instead of isl::size::release

patacca updated this revision to Diff 365769.Aug 11 2021, 9:01 AM

Move definition of polly::rangeIslSize outside the scope for debug builds

patacca edited the summary of this revision. (Show Details)Aug 11 2021, 9:26 AM

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.

polly/include/polly/Support/ISLTools.h
25

"Lint: Pre-merge checks" is correct, we use camelCase for functions. Maybe you also have an idea for a better name than my suggestion.

506

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?

607

[nit] unrelated whitespace change

polly/lib/Analysis/ScopBuilder.cpp
1165–1166

Use IslAssertNoError?

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

polly/include/polly/Support/ISLTools.h
506

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

polly/lib/Analysis/ScopBuilder.cpp
1165–1166

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

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?

Yes, that would be great.

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

It is not related to this patch, it failed before as well. See http://meinersbur.de:8011/#/builders/76/builds/337
(I added -mllvm -polly-process-unprofitable to the builder, then the failures showed up; It wasn't testing -mllvm -polly-process-unprofitable before that).

patacca updated this revision to Diff 366598.Aug 16 2021, 5:09 AM

Revert back to diff 7 (id 365038)

patacca edited the summary of this revision. (Show Details)Aug 16 2021, 5:11 AM

The patch has been tested with the llvm-test-suite and it is passing all the tests except for MultiSource/Applications/lambda-0.1.3/lambda.test but as previously stated the failure is not related to this patch.
If it looks good I will land this revision.

This revision is now accepted and ready to land.Aug 16 2021, 6:00 AM
This revision was landed with ongoing or failed builds.Aug 16 2021, 6:53 AM
This revision was automatically updated to reflect the committed changes.