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

1) fixes in loop partitioning algorithm 2) added a testcase #2956

Merged
merged 1 commit into from
Apr 26, 2019

Conversation

derisavi
Copy link
Contributor

@derisavi derisavi commented Apr 3, 2019

This fixes #2898. The main changes are:

  1. partitioning based on when a condition is false (in addition to when a condition is true)
  2. Recursively partition pre-subrange, middle subrange and post-subrange when necessary.

I did my best to keep the changes as small as possible, i.e., if we remove the code for any of the changes above, we won't generate correct code.

@tqchen @ZihengJiang please review.

@derisavi derisavi force-pushed the loop-partition branch 2 times, most recently from e67dbcc to 5c5bdf9 Compare April 3, 2019 17:45
@tqchen
Copy link
Member

tqchen commented Apr 4, 2019

@derisavi please also tag more reviewers

@derisavi
Copy link
Contributor Author

derisavi commented Apr 4, 2019

@anijain2305 @MarisaKirisame @xqdan @yzhliu Please review. Thanks.

Copy link
Contributor

@anijain2305 anijain2305 left a comment

Choose a reason for hiding this comment

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

Thanks @derisavi This is a very useful patch. Handling multiple variables in the same condition is very helpful.

Does it also solve the issue that you pointed out in #2898?

@derisavi
Copy link
Contributor Author

derisavi commented Apr 5, 2019

Handling multiple variables in the same condition is very helpful. Does it also solve the issue that you pointed out in #2898?

To be clear, the code before my change did handle multiple vars in the same condition. My change fixes other problems (e.g., in some cases we didn't recursively partition pre- or post-subranges, and we didn't partition based on when conditions are false).
One testcase that demonstrates the problem is given in #2898. Yes, this patch does resolve that problem.

src/pass/loop_partition.cc Show resolved Hide resolved
arith::Interval interval = kv.second.as<arith::IntervalSet>()->i;
auto intersection = arith::Interval::make_intersection(interval, for_interval);

// TODO(derisavi): the following if statement needs to be removed as soon as
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet. The commit of HalideIR currently used in master branch of tvm is 55ba177 which is older than a768f2f0 that is required to remove that if statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you update the submodule then remove here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to pick latest commit in HalideIR but apparently because of this commit (dmlc/HalideIR@92ef20f) , TVM source code compilation gives an error. I once tried to fix this compile problem but it wasn't trivial and required understanding the code change in HalideIR. (@jroesch can you please take a look and help us out?)

So we can either 1) wait for this compile problem to be fixed, perform the TODO, and then merge this PR or 2) merge this PR, wait for the compile problem to be fixed and then perform the TODO? I'm open to both but I prefer (2).

Copy link
Contributor

Choose a reason for hiding this comment

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

@derisavi Okay, let us merge the PR for now

Copy link
Contributor

Choose a reason for hiding this comment

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

@derisavi Could you try other commit like HEAD~1 besides the head

Copy link
Contributor Author

@derisavi derisavi Apr 26, 2019

Choose a reason for hiding this comment

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

The change I need is at HEAD and the problematic commit is HEAD~1 and TVM is currently using HEAD~2. So trying HEAD~1 wouldn't resolve the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rebased/squashed and pushed with the TODO item still in the code.

…sed when double splitting with indivisible factors 2) added a testcase
@ZihengJiang
Copy link
Contributor

Thanks @derisavi , this is merged

@ZihengJiang ZihengJiang merged commit 7e68d63 into apache:master Apr 26, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request May 13, 2019
…sed when double splitting with indivisible factors 2) added a testcase (apache#2956)
wweic pushed a commit to neo-ai/tvm that referenced this pull request May 13, 2019
…sed when double splitting with indivisible factors 2) added a testcase (apache#2956)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TEXPR][PASS] Loop distribution pass generates incorrect code
4 participants