Add % operator and propagate it from parser -> halide -> isl #348

prigoyal · 2018-04-25T20:05:41Z

adding % operator support

closes #290

abadams · 2018-04-25T20:11:17Z

test/cuda/test_execution_engine.cc

+  Check(
+      R"(
+def local_sparse_convolution(float(N, C, H, W) I, float(O, KC, KH, KW) W1) -> (O) {
+    O(n, o, h, w) +=! I(n, (c + kc) % c, h + kh, w + kw) * W1(o, kc, kh, kw) where c in 0:C


Isn't (c + kc) % c equal to (kc % c) ?

yeah, I just borrowed the example from what user posted in #290

it did feel a bit weird :D

abadams · 2018-04-25T20:11:47Z

This all makes sense to me so far.

ftynse · 2018-04-25T21:07:42Z

When you get to Halide -> isl conversion, there may be a need to differentiate between right hand sides of % that appear in index expressions. If those are compile-time constants, we can express them as quasi-affine expressions and have exact polyhedral analyses. Otherwise, we need to overapproximate like we do for indirections.

prigoyal · 2018-04-30T19:51:38Z

thanks @ftynse . I looked further and tried to understand the tc2halide and halide2isl better. It looks like the segfault happens because of infinite recursion in https://github.com/facebookresearch/TensorComprehensions/blob/master/tc/core/halide2isl.cc#L203 . I fixed that bug and test seems to pass now :)

prigoyal · 2018-04-30T20:23:28Z

tc/core/halide2isl.cc

    void visit(const Variable* op) {
      if (!included.count(op->name)) {
        if (op->param.defined()) {
+          // why is this empty? - what's the point of this?


I didn't quite understand why this is empty and added a comment. if one of the reviewers can explain it, that'd be great and I can remove this comment as well

prigoyal · 2018-04-30T20:23:51Z

tc/core/halide2isl.cc

    // We cannot span multiple constraints if a modulo operation is involved.
    // x > max(a,b) % C is not equivalent to (x > a % C && x > b % C).
-    auto lhs = makeIslAffBoundsFromExpr(space, e, false, false);
+    auto lhs = makeIslAffBoundsFromExpr(space, op->a, false, false);


using e would lead to infinite recursion and hence segfault

This comment should be in the commit message.

prigoyal · 2018-04-30T20:24:30Z

tc/core/tc2halide.cc

-        }
-        dims.push_back(Variable::make(Int(32), p.name(), p));
+  }
+  lang::TensorType type = p.tensorType();


there is no need for else here since we return from the if condition if satisfied. cc @abadams, hope this is okay?

prigoyal · 2018-04-30T20:25:30Z

tc/core/tc2halide.cc

+// funcs, bounds, reductions will be populated
 void translateComprehension(
-    const lang::Comprehension& c,
+    const lang::Comprehension& comprehension,


while reading the code, it was confusing to me to read c so I made it more descriptive to facilitate code readability. Hope this is okay, otherwise I can revert too.

+1, one character names are for iteration variables of short loops

prigoyal · 2018-04-30T20:26:24Z

tc/lang/sema.h

    }
    return matched_type;
  }
+


I was repeatedly getting confused about where a function started and ended. So I added these white lines to help. is this okay @zdevito ? :)

prigoyal · 2018-04-30T20:26:51Z

tc/lang/sema.h

+
  TreeRef floatType(TreeRef anchor) {
-    return c(TK_FLOAT, anchor->range(), {});
+    return createCompound(TK_FLOAT, anchor->range(), {});


simple c was confusing to me as a function name.

nicolasvasilache

I see at least 5 independent pieces here:

documentation improvements
legibility improvements
non-behavioral code changes (early exit if vs else)
change makeIslAffBoundsFromExpr(space, e, false, false); -> makeIslAffBoundsFromExpr(space, op->a, false, false);
support for %

Please separate those changes into independent commits with proper messages. In particular atm it is difficult to see the difference in the translateParam function between changes related to early exit and changes related to new behavior.

Thanks!

nicolasvasilache · 2018-05-01T15:02:13Z

test/cuda/test_execution_engine.cc

+  Check(
+      R"(
+def local_sparse_convolution(float(N, C, H, W) I, float(O, KC, KH, KW) W1) -> (O1) {
+    O1(n, o, h, w) +=! I(n, kc % c, h + kh, w + kw) * W1(o, kc, kh, kw) where c in 0:C


This cannot be correct, it will introduce kc % 0 operations.

nicolasvasilache · 2018-05-01T15:04:11Z

test/cuda/test_execution_engine.cc

+      tc::CudaMappingOptions::makeConvolutionCudaMappingOptions(),
+      inputs,
+      outputs);
+}


Please add an actual check function (even if you have to write it using plain C loops).

nicolasvasilache · 2018-05-01T15:05:43Z

tc/lang/sema.h

  }
-  TreeRef c(int kind, const SourceRange& range, TreeList&& trees) {
+
+  TreeRef createCompound(int kind, const SourceRange& range, TreeList&& trees) {


We have been using makeXYZ syntax throughout the codebase for functions that return an owning pointer, let's try to be consistent.

it was named create... because that's how the functions are named in lexer/sema/parser codebade to create a new node like: https://github.com/facebookresearch/TensorComprehensions/blob/master/tc/lang/tree_views.h#L204. The coding conventions recommend to have consistency locally within lexer/sema/parser (for example) code base here https://github.com/facebookresearch/TensorComprehensions/blob/master/CodingConventions.md#naming

if the naming should be make..., 1. can you kindly create a rule in CodingConventions.md so that people can follow that? 2. it would be also nice to define when should global/local rules be followed? thanks :)

sounds good, local trumps global :)

nicolasvasilache · 2018-05-01T15:05:47Z

tc/lang/sema.h

  }
-  TreeRef s(const std::string& s) {
+
+  TreeRef createString(const std::string& s) {


We have been using makeXYZ syntax throughout the codebase for functions that return an owning pointer, let's try to be consistent.

[copy-paste from above]:

it was named create... because that's how the functions are named in lexer/sema/parser codebade to create a new node like: https://github.com/facebookresearch/TensorComprehensions/blob/master/tc/lang/tree_views.h#L204. The coding conventions recommend to have consistency locally within lexer/sema/parser (for example) code base here https://github.com/facebookresearch/TensorComprehensions/blob/master/CodingConventions.md#naming

if the naming should be make..., 1. can you kindly create a rule in CodingConventions.md so that people can follow that? 2. it would be also nice to define when should global/local rules be followed? thanks :)

sounds good, local trumps global :)

createString doesn't appear to be used anywhere.
It may be better to simply remove it (in a separate commit).

comments have been addressed. Requesting again. Thanks.

nicolasvasilache · 2018-05-01T20:43:30Z

tc/core/tc2halide.cc

+      if (it != params->end()) {
+        p = it->second;
      } else {
-        CHECK(d_->kind() == lang::TK_CONST);


This hoisting looks like a behavior change to me, could you please explain why it is necessary or correlated with this commit message?

thanks for the review. I am not sure I understand. There is no code change to this section except removing unnecessary else after line 74.

line 88 - 98 in the green section matches line 85 - 95 in red section. I think that github alignment for code changes is not exactly good and hence causing confusion.

diff computation alogrithm is not perfect; reviewing changes per-commit helps

Ah yes, you're right, I see there are 2 columns of numbers. I learned something today! Sorry for the noise here

nicolasvasilache · 2018-05-01T21:05:51Z

So it seems the unit test change requested was addressed by simply deleting the unit test.
As it stands, this PR adds % to parsing and checks that the operation is indeed performed on an int32 value which is good.

However as far as I see, it does not address #290 which requires support for modulus array indexing. So I would remove the halide -> isl part from your PR title until this is addressed and a proper test is added.

Could you elaborate on what happens if you try to compile a TC with a modulus array indexing? I assume the parsing now passes but I would expect some undefined behavior when reaching ISL.

Does the TC mapper complain in an actionable way, just segfault/crash, produce some wrong code or something else?

prigoyal · 2018-05-02T20:35:58Z

test/test_cuda_mapper.cc

+)TC";
+  // This triggers tc2halide conversion and should not throw.
+  auto scop = Prepare(tc);
+  auto reads = scop->reads;


hey @ftynse , I have extracted out the reads here but I don't know how to get the isl stride information. I tried to search in the codebase but couldn't find much help. Can you give some pointers? thanks.

Yeah, you probably want to search in the third-party/islpp as well :) There is a greppable documentation file in doc/.

Something along the lines of

for (auto r : scop->reads.range()) { if (r.get_tuple_name() != std::string("B")) { // skip irrelevant reads, if any continue; } EXPECT_EQ(r.get_stride(), putExpectedStrideValueHere); // EXPECT_TRUE(r.plain_is_universe()); // for the the set being universe }

should work

thanks. I tried the above snippet but getting compilation failure on for (auto r : scop->reads.range()) it seems like it's not iterable. The error looks like

error: no matching function for call to ‘begin(isl::union_set&)’

is the https://github.com/simbuerg/isl/blob/master/doc/user.pod correct file to consult?

Sorry, my bad, try scop->reads.range().get_set_list() instead.

This one https://github.com/nicolasvasilache/isl/blob/ntv_dev/doc/user.pod comes from what we submodule (our copy of isl is ahead even of isl's master, and the link you posted is not master)

addressed all feedback so far. requesting guidance on unit test added in test_cuda_mapper

zdevito

Frontend changes look good to me. I didn't catch where in halide2isl that % was added. Did it previously exist?

prigoyal · 2018-05-02T21:40:26Z

the % operator existed in halide2isl but it was segfaulting. 52989ea commit fixed that :)

skimo-openhub · 2018-05-03T07:48:12Z

On Wed, May 02, 2018 at 09:07:31AM -0700, Priya Goyal wrote: prigoyal commented on this pull request. > @@ -339,6 +339,21 @@ struct Sema { throw ErrorReport(exp) << "NYI - semantic checking for " << exp; } } + // This is the entry function for semantic analysis. It is called by + // tc2halide to associate type with each node of the tree and to also make + // sure that the tree is sematically correct. For example: a variable + // may not be input two times. Parser only verifies for the syntax but does + // not check the semantics. + // + // It converts the TK_APPLY nodes to TK_ACCESS or TK_BUILT_IN + // + // The reduction variables are deduced and the objects are created for them + // and they are appended to the tree + // + // Type checking is also done but by a very small codebase + // + // withType - associates the type with a given node checkFunction is the main entry point for Sema codebase called by `tc2halide` I added `withType` because this is important component of sema where each node gets it's proper type. I wanted to highlight that it's an important functionality as part of what `checkFunction` will result it.

Hmm... now it just says "Associates the type with a given node". Is this what checkFunction does or what withType does? You added that to the documentation of checkFunction. If you want to refer to some method from inside the documenation of another method, then please be a bit more explicit about it. "The method withType can be used to ..." Just reading this "withType -", I really had no idea what it was about. skimo

prigoyal · 2018-05-03T14:12:58Z

test/test_cuda_mapper.cc

+    if (r.get_tuple_name() != std::string("a")) {
+      continue;
+    }
+    // EXPECT_EQ(r.get_stride(?), ?);


I am not quite sure what should be the pos for get_stride().

index of the dimension in the set (position of the subscript that should involve the stride), 0 in this case I believe

ftynse · 2018-05-03T22:12:41Z

test/test_cuda_mapper.cc

+    if (r.get_tuple_name() != std::string("a")) {
+      continue;
+    }
+    // EXPECT_EQ(r.get_stride(0), ?);


this test is still commented out...

ftynse · 2018-05-04T22:22:06Z

test/test_cuda_mapper.cc

+)TC";
+  // This triggers tc2halide conversion and should not throw.
+  auto scop = Prepare(tc);
+  for (auto r : scop->reads.range().get_set_list()) {


Sorry, I misled you here; taking the range of the access relation here will actually drop the stride because the stride is on the input dimension. Instead, let's use wrapping. It is a transformation that reshapes the space from a relation ([i]->a[a0]) to a set wrapping a relation ([[i]->a[a0]], note the extra surrounding [] that mean it's a set now). In that set, we can take the stride an compare it with the expected value. However, we cannot directly check the ex-range's name, so we need to unwrap the set and take its range before using get_tuple_name().

Now that you have an idea of how isl is structured, you should be able to find the (un)wrapping functions and use them properly :)

thanks @ftynse . can you provide guidance on skimo's comment ? I am not sure myself which is the right approach. Thanks a lot :)

It's a matter of preference, I think. @skimo-openhub suggests to construct a new relation from its textual description, basically isl::map(ctx, "<paste here the textual representation of the map litearlly>") (taking ctx from whatever isl object available around) and comparing that to the result. It would work, but I prefer testing only the relevant properties of the relation, hence the stride check. Checking only relevant properties removes the need to modify the test when irrelevant properties change. I will accept both ways of checking.

More specifically, current access relations do not involve bounds imposed by tensor sizes. I'm quite annoyed at this during memory promotion and was planning to add those bounds in access relation construction. That would change these relations, requiring to update this test that is affected only indirectly.

skimo-openhub · 2018-05-07T10:59:54Z

On Fri, May 04, 2018 at 10:22:09PM +0000, ftynse wrote: ftynse commented on this pull request. > @@ -753,6 +753,38 @@ def perforatedConvolution(float(N, C, H, W) input, float(M, C, KH, KW) weights, Prepare(tc); } +TEST_F(PolyhedralMapperTest, ModulusConstantRHS) { + string tc = R"TC( +def fun(float(N) a) -> (b) { b(i) = a(i % 3) where i in 0:N } +)TC"; + // This triggers tc2halide conversion and should not throw. + auto scop = Prepare(tc); + for (auto r : scop->reads.range().get_set_list()) { Sorry, I misled you here; taking the range of the access relation here will actually drop the stride because the stride is on the input dimension. Instead, let's use _wrapping_. It is a transformation that reshapes the space from a relation (`[i]->a[a0]`) to a set _wrapping a relation_ (`[[i]->a[a0]]`, note the extra surrounding `[]` that mean it's a set now). In that set, we can take the stride an compare it with the expected value. However, we cannot directly check the ex-range's name, so we need to unwrap the set and take its range before using `get_tuple_name()`.

Why do you see this as a stride check? Why don't you simply compare the access relation to the expected access relation (perhaps after projecting out the reference tag)? skimo

skimo-openhub · 2018-05-09T10:37:17Z

On Tue, May 08, 2018 at 01:30:16PM -0700, ftynse wrote: ftynse commented on this pull request. > @@ -753,6 +753,38 @@ def perforatedConvolution(float(N, C, H, W) input, float(M, C, KH, KW) weights, Prepare(tc); } +TEST_F(PolyhedralMapperTest, ModulusConstantRHS) { + string tc = R"TC( +def fun(float(N) a) -> (b) { b(i) = a(i % 3) where i in 0:N } +)TC"; + // This triggers tc2halide conversion and should not throw. + auto scop = Prepare(tc); + for (auto r : scop->reads.range().get_set_list()) { It's a matter of preference, I think. @skimo-openhub suggests to construct a new relation from its textual description, basically `isl::map(ctx, "<paste here the textual representation of the map litearlly>")` (taking `ctx` from whatever isl object available around) and comparing that to the result. It would work, but I prefer testing only the relevant properties of the relation, hence the stride check. Checking only relevant properties removes the need to modify the test when irrelevant properties change. I will accept both ways of checking.

Of course you should only check for relevant properties, but the stride is only a part of the property you want to check. In any case, don't do any wrapping. We can easily add a isl_map_get_range_stride based on isl_map_get_range_stride_info in the same way isl_set_get_stride is based on isl_set_get_stride_info and then use isl_map_get_range_stride.

More specifically, current access relations do not involve bounds imposed by tensor sizes. I'm quite annoyed at this during memory promotion and was planning to add those bounds in access relation construction. That would change these relations, requiring to update this test that is affected only indirectly.

You shouldn't check that is _equal_ to some map, but a _subset_ of the expected property (and perhaps also that it is non-empty). skimo

ftynse · 2018-06-06T21:55:30Z

rebased and revived

the 'if' condition is checked and 'return' happens if the condition is met. Using 'else' is not needed

proper variable naming and whitelining between functions

change makeIslAffBoundsFromExpr(..., e, ...) to makeIslAffBoundsFromExpr(..., op->a, ...) to avoid infinite recursion leading to segfault

support % operator: propagate it from parser to halide to isl and add unit tests

dead function and not used anywhere so cleaning it up

prigoyal added the wip label Apr 25, 2018

prigoyal requested review from abadams, skimo-openhub and zdevito April 25, 2018 20:05

facebook-github-bot added the CLA Signed label Apr 25, 2018

prigoyal changed the title ~~[wip] Add modulo operator and propagate it from parser to halide to isl~~ [wip] Add modulo operator and propagate it from parser -> halide -> isl Apr 25, 2018

prigoyal changed the title ~~[wip] Add modulo operator and propagate it from parser -> halide -> isl~~ [wip] Add % operator and propagate it from parser -> halide -> isl Apr 25, 2018

abadams reviewed Apr 25, 2018

View reviewed changes

prigoyal force-pushed the add-operators branch from 2097635 to e389bc9 Compare April 30, 2018 14:01

prigoyal force-pushed the add-operators branch from e389bc9 to bb9fe44 Compare April 30, 2018 19:52

prigoyal changed the title ~~[wip] Add % operator and propagate it from parser -> halide -> isl~~ Add % operator and propagate it from parser -> halide -> isl Apr 30, 2018

prigoyal added review me and removed wip labels Apr 30, 2018

prigoyal force-pushed the add-operators branch 2 times, most recently from 5662537 to cad7cf3 Compare April 30, 2018 19:57

prigoyal requested a review from ftynse April 30, 2018 20:02

prigoyal commented Apr 30, 2018

View reviewed changes

prigoyal force-pushed the add-operators branch from cad7cf3 to a44e7df Compare May 1, 2018 14:08

nicolasvasilache previously requested changes May 1, 2018

View reviewed changes

nicolasvasilache reviewed May 1, 2018

View reviewed changes

prigoyal force-pushed the add-operators branch from a44e7df to a57562a Compare May 1, 2018 20:30

nicolasvasilache reviewed May 1, 2018

View reviewed changes

prigoyal commented May 2, 2018

View reviewed changes

zdevito previously approved these changes May 2, 2018

View reviewed changes

prigoyal dismissed zdevito’s stale review via 0960f03 May 3, 2018 13:45

prigoyal force-pushed the add-operators branch 2 times, most recently from 0960f03 to 022c62a Compare May 3, 2018 14:12

prigoyal commented May 3, 2018

View reviewed changes

prigoyal force-pushed the add-operators branch 2 times, most recently from 8e390f3 to 34bd0b1 Compare May 3, 2018 17:41

ftynse reviewed May 3, 2018

View reviewed changes

ftynse reviewed May 4, 2018

View reviewed changes

ftynse force-pushed the add-operators branch from 67e381c to 1402495 Compare June 6, 2018 21:54

ftynse force-pushed the add-operators branch from 1402495 to 8af6370 Compare June 7, 2018 21:26

ftynse previously approved these changes Jun 7, 2018

View reviewed changes

ftynse dismissed their stale review via ab72ede June 7, 2018 21:55

ftynse force-pushed the add-operators branch from 8af6370 to ab72ede Compare June 7, 2018 21:55

prigoyal added 6 commits June 8, 2018 07:57

add some documentation to sema/lexer/parser and tc2halide

e143ff2

remove unnecessary else in if-else given early exit

71539d1

the 'if' condition is checked and 'return' happens if the condition is met. Using 'else' is not needed

improve code readability

8d0781c

proper variable naming and whitelining between functions

fix segfault in halide2isl for mod op

fb956bd

change makeIslAffBoundsFromExpr(..., e, ...) to makeIslAffBoundsFromExpr(..., op->a, ...) to avoid infinite recursion leading to segfault

add support for modulo operator

6e9da0d

support % operator: propagate it from parser to halide to isl and add unit tests

sema createString cleanup

b36f466

dead function and not used anywhere so cleaning it up

nicolasvasilache force-pushed the add-operators branch from ab72ede to b36f466 Compare June 8, 2018 13:59

nicolasvasilache approved these changes Jun 8, 2018

View reviewed changes

nicolasvasilache merged commit 702d7f5 into master Jun 8, 2018

Uh oh!

Add % operator and propagate it from parser -> halide -> isl #348

Add % operator and propagate it from parser -> halide -> isl #348

Uh oh!

Conversation

prigoyal commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abadams commented Apr 25, 2018

Uh oh!

ftynse commented Apr 25, 2018

Uh oh!

prigoyal commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prigoyal Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasvasilache left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasvasilache May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasvasilache commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prigoyal May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ftynse May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

prigoyal commented Apr 25, 2018 •

edited

Loading

prigoyal Apr 30, 2018 •

edited

Loading

nicolasvasilache May 1, 2018 •

edited

Loading

nicolasvasilache commented May 1, 2018 •

edited

Loading

prigoyal May 2, 2018 •

edited

Loading

ftynse May 2, 2018 •

edited

Loading

ftynse May 3, 2018 •

edited

Loading