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

[Bug] [opt] Fix compilation crash when there is a container statement after an unconditional continue #1299

Merged
merged 4 commits into from
Jun 22, 2020

Conversation

xumingkuan
Copy link
Contributor

@xumingkuan xumingkuan commented Jun 21, 2020

Related issue = fix #1297

I moved the unreachable code elimination pass from cfg_optimization to simplify because the former can't eliminate container statements properly. I tried writing something like std::vector<Stmt *> not_in_any_nodes in the CFG, but it's hard to know whether a ContinueStmt can be eliminated in the CFG, so an IfStmt with only one ContinueStmt in it can't be eliminated easily too.

[Click here for the format server]


location = (int)statements.size() - 1;
location = (int)statements.size();
Copy link
Contributor Author

@xumingkuan xumingkuan Jun 21, 2020

Choose a reason for hiding this comment

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

(I wonder why this has been in the codebase for 5 months...)

So actually it should be

            if a:
                continue
            if a:
                if a:
                    continue

instead of

            if a:
                if a:
                    continue
            if a:
                continue

which causes ControlFlowGraph::unreachable_code_elimination to produce malformed IR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing this! It's surprising that we have this issue for so long time without noticing...

@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #1299 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1299   +/-   ##
=======================================
  Coverage   85.59%   85.59%           
=======================================
  Files          18       18           
  Lines        3283     3283           
  Branches      621      621           
=======================================
  Hits         2810     2810           
  Misses        345      345           
  Partials      128      128           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 470dc97...bebc0dd. Read the comment docs.

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

LGTM, thank you so much!

location = (int)statements.size() - 1;
location = (int)statements.size();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing this! It's surprising that we have this issue for so long time without noticing...

@xumingkuan xumingkuan merged commit 824c94b into taichi-dev:master Jun 22, 2020
@xumingkuan xumingkuan deleted the continue branch June 23, 2020 19:46
@FantasyVR FantasyVR mentioned this pull request Jun 24, 2020
Rullec pushed a commit to Rullec/taichi that referenced this pull request Jun 26, 2020
… after an unconditional continue (taichi-dev#1299)

* [Bug] [opt] Fix compilation crash when there is a container statement after an unconditional continue

* [skip ci] enforce code format

* retrigger CI

* retrigger CI

Co-authored-by: Taichi Gardener <[email protected]>
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.

[Bug] [ir] a simple kernel with multiple continue fails to compile
3 participants