[WIP] use templated isl types #533

skimo-openhub · 2018-06-21T10:57:54Z

Templated isl types require the user to specify the domain and range
universes of isl objects, allowing the compiler to check whether
it makes sense to combine pairs of objects.
This RFC only converts isPromotableToRegistersBelow and some related
functions to illustrate the effect.
The isPromotableToRegistersBelow was already applying operations
correctly, so the code itself did not require any changes.
However, one variable was reused to store different types
of intermediate result and this one had to be split up into
several variables because they now have different types.

nicolasvasilache · 2018-06-21T18:51:07Z

From a quick glance, this looks great to me

skimo-openhub · 2018-06-21T10:58:31Z

tc/core/polyhedral/cuda/memory_promotion_heuristic.cc

@@ -365,7 +371,7 @@ bool accessSubscriptsAreUnrolledLoops(
    auto subdomain = activeDomainPointsBelow(root, leaf);

    auto unrolledDims = isl::union_pw_aff_list(leaf->ctx_, 1);
-    for (auto node : ancestors) {
+    for (const detail::ScheduleTree* node : ancestors) {


It's not immediately obvious why this had to be changed, but leaving in auto results in

home/skimo/git/c2isl/tc/core/polyhedral/cuda/memory_promotion_heuristic.cc:375:60: error: expected primary-expression before ‘>’ token auto band = node->elemAs<detail::ScheduleTreeElemBand>();

try (I think):

auto band = node.template operator->(elemAs<detail::ScheduleTreeElemBand>());

Not sure if it works I seem to remember using that with operator.

or what @ftynse explained :)

skimo-openhub · 2018-06-22T09:03:47Z

tc/core/polyhedral/cuda/memory_promotion_heuristic.cc

@@ -365,7 +371,7 @@ bool accessSubscriptsAreUnrolledLoops(
    auto subdomain = activeDomainPointsBelow(root, leaf);

    auto unrolledDims = isl::union_pw_aff_list(leaf->ctx_, 1);
-    for (auto node : ancestors) {
+    for (const detail::ScheduleTree* node : ancestors) {
      auto band = node->elemAs<detail::ScheduleTreeElemBand>();


Writing this as auto band = node->template elemAs<detail::ScheduleTreeElemBand>(); allows me to keep the auto, but it is again not obvious why I need to introduce template here.

Syntactic disambiguation. A particularly nasty interplay between templates and type inference. The syntax of a template is checked when parsing its definition. But type inference for auto is performed at template instantiation time. When parsing elemAs<detail::ScheduleTreeElemBand, the compiler wouldn't know if < is a comparison operator or the start of a template argument list. Unless elemAs is known to be a template (but it is not known because it's type has not been inferred yet when node is declared as auto), < is treated as a comparison operator. In this case, the RHS must be an expression. detail::ScheduleTreeElemBand>() is not a valid expression so the compiler complains. Adding the template prefix indicates that < must be always treated as the start of the template argument list, so it fixes the problem. So does manually specifying the type of node because the compiler now knows that elemAs is a function template.

skimo-openhub · 2018-06-22T12:12:31Z

On Fri, Jun 22, 2018 at 04:52:38AM -0700, ftynse wrote: Syntactic disambiguation. A particularly nasty interplay between templates and type inference. The syntax of a template is checked when parsing its definition. But type inference for `auto` is performed at template instantiation time. When parsing `elemAs<detail::ScheduleTreeElemBand`, the compiler wouldn't know if `<` is a comparison operator or the start of a template argument list. Unless `elemAs` is known to be a template (but it is not known because it's type has not been inferred yet when `node` is declared as `auto`), `<` is treated as a comparison operator. In this case, the RHS must be an expression. `detail::ScheduleTreeElemBand>()` is not a valid expression so the compiler complains. Adding the `template` prefix indicates that `<` must be always treated as the start of the template argument list, so it fixes the problem. So does manually specifying the type of `node` because the compiler now knows that `elemAs` is a function template.

Thanks. It makes sense now. skimo

ftynse · 2018-06-29T12:50:53Z

I like the general idea. What worries me is the need to manually introduce member functions in the class templates. Those in isl bindings are autogenerated...

skimo-openhub · 2018-06-29T12:53:48Z

On Fri, Jun 29, 2018 at 05:50:56AM -0700, ftynse wrote: I like the general idea. What worries me is the need to manually introduce member functions in the class templates. Those in isl bindings are autogenerated...

The idea would be for us to experiment with this for a while and if it turns out to be useful, we can think of autogenerating these functions too. Note that this would require some additional annotations. skimo

facebook-github-bot · 2018-07-25T21:09:35Z

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.

facebook-github-bot · 2018-07-26T15:58:44Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

skimo-openhub · 2018-07-26T16:02:47Z

On Fri, Jun 29, 2018 at 05:50:56AM -0700, ftynse wrote: I like the general idea. What worries me is the need to manually introduce member functions in the class templates. Those in isl bindings are autogenerated...

I'm now generating these as well, but I'm still trying to pick the pieces apart, so it's not quite ready yet. skimo

This allows outputRanges to be converted to templated isl types without having to convert TensorReferenceGroup::promotion at the same time. Automatic type deduction will be reintroduced when TensorReferenceGroup::promotion is converted.

Since removeRangeStrides has not been converted yet, a separate variable is introduced to store its result. Since ScopedFootprint::strideOffsets is modified, TensorReferenceGroup::promotion needs some modifications too.

This allows TensorReferenceGroup.originalAccesses() to be converted to templated isl types without having to convert accessSubscriptsAreUnrolledLoops at the same time. Automatic type deduction will be reintroduced when accessSubscriptsAreUnrolledLoops is converted.

skimo-openhub · 2018-09-01T12:30:05Z

I got kicked out of the GitHub Facebook organization yesterday (before the end of my working day),
so I can't update this PR. I created a new one instead (#604).

nicolasvasilache

trying to unsubscribe, don't see a way other than approving

skimo-openhub requested review from ftynse and nicolasvasilache June 21, 2018 10:57

facebook-github-bot added the CLA Signed label Jun 21, 2018

skimo-openhub commented Jun 22, 2018

View reviewed changes

skimo-openhub changed the base branch from pr/clean_up to master June 22, 2018 11:01

skimo-openhub force-pushed the pr/template branch from 2a546c8 to 22cf55d Compare June 25, 2018 11:33

skimo-openhub added the wip label Jul 26, 2018

skimo-openhub self-assigned this Jul 26, 2018

skimo-openhub changed the title ~~[RFC] use templated isl types~~ [WIP] use templated isl types Jul 26, 2018

skimo-openhub force-pushed the pr/template branch from 22cf55d to 835035f Compare July 26, 2018 15:59

skimo-openhub force-pushed the pr/template branch 12 times, most recently from 340ddb8 to 57e4ceb Compare August 1, 2018 10:00

Sven Verdoolaege added 27 commits August 3, 2018 12:35

makeSingleton: use templated isl types

a6fac0d

TensorReference: use templated isl types

6ca1fa6

tensorElementsSet: use templated isl types

b6fae4d

promotedFootprint: use templated isl types

f021617

outputRanges: use templated isl types

28de59d

Since removeRangeStrides has not been converted yet, a separate variable is introduced to store its result. Since ScopedFootprint::strideOffsets is modified, TensorReferenceGroup::promotion needs some modifications too.

removeRangeStrides: use templated isl types

5687150

ScopedFootprint.box: use templated isl types

1dfc84e

TensorReferenceGroup::promotion: use templated isl types

5b18188

use templated isl types in schedule tree

08bc1a2

MappedScop::map: use templated isl types

125cdb5

makeMapping: use templated isl types

23a82a7

infixScheduleMupa: use templated isl types

f9ffa2b

MappedScop::findBestSync: use templated isl types

26d5f4a

Scop::domainRef: use templated isl types

cae6a9a

promoteToRegistersBelow: use templated isl types

589ba9e

partialScheduleMupa: use templated isl types

84d1363

activeDomainPointsHelper: use templated isl types

b296604

TensorReferenceGroup::originalAccesses use templated isl types

b7f2fcc

makeNextElementMap: use templated isl types

fea7e37

hasReuseWithin: use templated isl types

78c61db

isPromotableToRegistersBelow: use templated isl types

4b8db9f

accessSubscriptsAreUnrolledLoops: use templated isl types

6440b03

collectMappingsTo: use templated isl types

229a698

TensorReferenceGroup::approximateScopedAccesses: use templated isl types

d0e4d1b

insertCopiesUnder: use templated isl types

dbc7391

skimo-openhub force-pushed the pr/template branch from 0b3446d to dbc7391 Compare August 3, 2018 11:00

nicolasvasilache approved these changes Dec 11, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] use templated isl types #533

[WIP] use templated isl types #533

skimo-openhub commented Jun 21, 2018

nicolasvasilache commented Jun 21, 2018

skimo-openhub Jun 21, 2018

nicolasvasilache Jun 22, 2018

nicolasvasilache Jun 22, 2018

skimo-openhub Jun 22, 2018

ftynse Jun 22, 2018

skimo-openhub commented Jun 22, 2018 via email

ftynse commented Jun 29, 2018

skimo-openhub commented Jun 29, 2018 via email

facebook-github-bot commented Jul 25, 2018

facebook-github-bot commented Jul 26, 2018

skimo-openhub commented Jul 26, 2018 via email

skimo-openhub commented Sep 1, 2018

nicolasvasilache left a comment

[WIP] use templated isl types #533

Are you sure you want to change the base?

[WIP] use templated isl types #533

Conversation

skimo-openhub commented Jun 21, 2018

nicolasvasilache commented Jun 21, 2018

skimo-openhub Jun 21, 2018

Choose a reason for hiding this comment

nicolasvasilache Jun 22, 2018

Choose a reason for hiding this comment

nicolasvasilache Jun 22, 2018

Choose a reason for hiding this comment

skimo-openhub Jun 22, 2018

Choose a reason for hiding this comment

ftynse Jun 22, 2018

Choose a reason for hiding this comment

skimo-openhub commented Jun 22, 2018 via email

ftynse commented Jun 29, 2018

skimo-openhub commented Jun 29, 2018 via email

facebook-github-bot commented Jul 25, 2018

facebook-github-bot commented Jul 26, 2018

skimo-openhub commented Jul 26, 2018 via email

skimo-openhub commented Sep 1, 2018

nicolasvasilache left a comment

Choose a reason for hiding this comment