Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Relay] Move prelude to text format #3939

Merged
merged 15 commits into from
Sep 29, 2019

Conversation

weberlo
Copy link
Contributor

@weberlo weberlo commented Sep 11, 2019

This PR is a follow-up to #3863, which replaces the hand-constructed ASTs in the current prelude with equivalent definitions in the text format.

One question I have about syntax is how we should handle zero-arity constructors. Most functional languages desugar, for example, Nil into Nil(), and I've implemented that functionality here. Let me know if that's not what we want.

CC @MarisaKirisame @slyubomirsky @jroesch @junrushao1994

@@ -18,12 +18,290 @@
*/
v0.0.4

def @id[a](%x: a) -> a {
%x
// TODO(weberlo): should we add sugar for scalar types (e.g., `int32` => `Tensor[(), int32]`)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, though it probably merits wider discussion

@jroesch
Copy link
Member

jroesch commented Sep 13, 2019

cc @zhiics and @wweic

@zhiics
Copy link
Member

zhiics commented Sep 13, 2019

Will do over the weekend.

@weberlo weberlo force-pushed the move-prelude-to-text-format branch from 58be955 to 67302e9 Compare September 15, 2019 21:37
@@ -151,6 +151,22 @@ void ModuleNode::Add(const GlobalVar& var,
AddUnchecked(var, checked_func);
}

void ModuleNode::AddUnchecked(const GlobalVar& var,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this function the same as before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. I just moved it next to Add for readability. I can move it back if you want.

src/relay/ir/module.cc Outdated Show resolved Hide resolved
include/tvm/relay/module.h Outdated Show resolved Hide resolved
Copy link
Contributor

@wweic wweic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll approve once CI is fixed.

@weberlo
Copy link
Contributor Author

weberlo commented Sep 27, 2019

The CI failure was because the VM compiler can't compile programs which make use of first-class constructors. For example,

/*
 * Concatenates two lists.
 */
def @concat[A](%xs: List[A], %ys: List[A]) -> List[A] {
  @foldr(Cons, %ys, %xs)
}

needs to be rewritten as

/*
 * Concatenates two lists.
 */
def @concat[A](%xs: List[A], %ys: List[A]) -> List[A] {
  let %updater = fn(%x: A, %xss: List[A]) -> List[A] {
    Cons(%x, %xss)
  };
  @foldr(%updater, %ys, %xs)
}

, in order to successfully compile.

@jroesch We do want first-class constructors, right? If so, I'll submit a follow-up PR that adds that functionality.

@weberlo
Copy link
Contributor Author

weberlo commented Sep 29, 2019

@wweic CI is green

@jroesch
Copy link
Member

jroesch commented Sep 29, 2019

The CI failure was because the VM compiler can't compile programs which make use of first-class constructors. For example,

/*
 * Concatenates two lists.
 */
def @concat[A](%xs: List[A], %ys: List[A]) -> List[A] {
  @foldr(Cons, %ys, %xs)
}

needs to be rewritten as

/*
 * Concatenates two lists.
 */
def @concat[A](%xs: List[A], %ys: List[A]) -> List[A] {
  let %updater = fn(%x: A, %xss: List[A]) -> List[A] {
    Cons(%x, %xss)
  };
  @foldr(%updater, %ys, %xs)
}

, in order to successfully compile.

@jroesch We do want first-class constructors, right? If so, I'll submit a follow-up PR that adds that functionality.

We should handle this with a full eta-expansion pass, I will try to find some resources on performing eta-expansion for these types of purposes.

@jroesch jroesch merged commit 2dac17d into apache:master Sep 29, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 30, 2019
* Fix parser

* Doc fix

* Add module utility functions necessary for prelude

* Implement prelude in text format

* Remove programmatically constructed prelude defs

* Fix 0-arity type conses in pretty printer and test

* Make prelude loading backwards-compatible

* Fix patterns

* Improve some prelude defs

* Fix `ImportFromStd`

It needs to also follow the "add unchecked, add checked" pattern

* Lint roller

* Woops

* Address feedback

* Fix `test_list_constructor` VM test

* Fix `test_adt.py` failures
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 30, 2019
* Fix parser

* Doc fix

* Add module utility functions necessary for prelude

* Implement prelude in text format

* Remove programmatically constructed prelude defs

* Fix 0-arity type conses in pretty printer and test

* Make prelude loading backwards-compatible

* Fix patterns

* Improve some prelude defs

* Fix `ImportFromStd`

It needs to also follow the "add unchecked, add checked" pattern

* Lint roller

* Woops

* Address feedback

* Fix `test_list_constructor` VM test

* Fix `test_adt.py` failures
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 1, 2019
* Fix parser

* Doc fix

* Add module utility functions necessary for prelude

* Implement prelude in text format

* Remove programmatically constructed prelude defs

* Fix 0-arity type conses in pretty printer and test

* Make prelude loading backwards-compatible

* Fix patterns

* Improve some prelude defs

* Fix `ImportFromStd`

It needs to also follow the "add unchecked, add checked" pattern

* Lint roller

* Woops

* Address feedback

* Fix `test_list_constructor` VM test

* Fix `test_adt.py` failures
petrex added a commit to petrex/tvm that referenced this pull request Oct 29, 2019
* master:
  Fix split's last factor issue (apache#4044)
  [COMMUNITY] ajtulloch -> committer (apache#4043)
  [TOPI]Add op argwhere (apache#3994)
  [topi] add ARM v8.2 udot (uint8) support (apache#3978)
  [COMMUNITY] anijain2305 -> reviewer (apache#4036)
  [QNN] Renaming dense operator. (apache#4033)
  [Relay][Compile_engine] Int64 shape handling for outputs. (apache#4031)
  Add dmlc-core to the list of installed header directories. (apache#4035)
  [ARITH] migrate indexdiv/mod to floordiv/mod (apache#4008)
  [Relay] Move prelude to text format (apache#3939)
  make tvm compilable by gcc 4.9.2 (apache#4032)
  [AUTOTVM][DOCS] Add a link to the defining network description of auto-tuning tutorial (apache#4023)
  [ARITH] cleanup the indexmod/div on python side (apache#4028)
  [Fix] Add more pad_mode support for onnx converter (apache#4029)
  Add parser support for ReLU tflite operator (apache#4022)
  Additional MXNet Convolution and Deconvolution tests (apache#4026)
  docs: minor spelling tweaks (apache#4027)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants