This repository was archived by the owner on Apr 28, 2023. It is now read-only.
[WIP] Exp/rnn #572
Open
[WIP] Exp/rnn #572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicolasvasilache
force-pushed
the
exp/rnn
branch
8 times, most recently
from
a116505 to
0c14a66
Compare
This PR temporarily introduces a Halide hack to make progress. After a quick exchange with @abadams this seems unnecessary as the use case for sequential dependencies probably only requires an RDom instead of a Var for the `t` for-loop index. Punting for now.
This commit adds a `for t in 0:T { ... }` syntax to the TC language.
Because it introduces an extension to the grammar and type changes,
modifications need to propagate all the way to `tc2halide`.
Support for loops is only implemented in the parser and semantic checker.
Emitting proper HalideIR and ScheduleTree will be implemented in follow-up
commits.
The commit should be reviewed in this order:
1. add support to the lexer
2. add the `For` compound type in `tree_views.h`
3. support `parseStmt` in the parser (in particular see how we
`parseRangeConstraint` first and reuse its index)
4. perform semantic checks that guarantee only a single nested
`For` looFor` is allowed (in parsee how we `checkRangeConstraint`
after checking all comprehensions)
5. update `tc2halide`
6. add new tests
This commit would crash if trying to emit HalideIR with `for` loops
but in order to compile we still needs to update `tc2halide`.
Disclaimer: this is WIP and does not work yet. In particular the ScheduleTree generated is incorrect (not yet properly scoped under for loop). Additionally, the example `For4InvalidHalide` seems to hit deeper structural issues of the high-level HalideIR that traditional polyhedral dependence analysis has no issue with (cc @abadams). This commit tests that the for-loop language construct properly propagates to the Halide bounds inference. This commit also explicitly adds checks that only a single RangeConstraint s`traiindex in min:max`) is used for each index. This requirement is added because it would be very surprising to mix explicit ranges coming from for loops with where clauses in a comprehension. In particular, the current behavior in Halide inference is to only keep the last RangeConstraint which systematically discards the information specified in the loop. `tc2halide` even says: ```// TODO: What if subsequent updates have incompatible bounds // (e.g. an in-place stencil)?. The .bound directive will use the // bounds of the last stage for all stages. ``` As a consequence we add an explicit check that multiple bounds specifications may not coexist.
nicolasvasilache
force-pushed
the
exp/rnn
branch
from
0c14a66 to
1f60110
Compare
|
Had a nice chat with @abadams and he'll be taking the baton on the Halide -> ISL portion as pieces of the code can be significantly simplified. |
|
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 cla@fb.com with your details so we can update your status. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WIP cc @apaszke