Skip to content

Conversation

dreiss
Copy link
Contributor

@dreiss dreiss commented Apr 23, 2020

Stack from ghstack:

Every one of these branches returns or raises, so there's no need for elif.
This makes it a little easier to reorder and move conditions.

Differential Revision: D21209992

Differential Revision: D21209992

Every one of these branches returns or raises, so there's no need for elif.
This makes it a little easier to reorder and move conditions.

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992/)

[ghstack-poisoned]
@dr-ci
Copy link

dr-ci bot commented Apr 23, 2020

💊 CI failures summary and remediations

As of commit 261ef6d (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 18 times.

Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

this is a reminder for me to do something that has been on my list but hasn't been pressing yet for any particular reason.
If you have a function like:

@torch.jit.script
def test_foo(x: bool, y: bool):
    if x:
        return 1
    return 2
print(test_foo.code)

generates:

def test_foo(x: bool,
    y: bool) -> int:
  _0 = uninitialized(int)
  if x:
    _1, _2 = True, 1
  else:
    _1, _2 = False, _0
  if _1:
    _3 = _2
  else:
    _3 = 2
  return _3

while

@torch.jit.script
def test_foo(x: bool, y: bool):
    if x:
        return 1
    else:
        return 2
print(test_foo.code)

generates:

def test_foo(x: bool,
    y: bool) -> int:
  if x:
    _0 = 1
  else:
    _0 = 2
  return _0

The reason for this is how we destructure early returns into if statements. I can get around to adding a rewrite pass sometime next week, but before then we probably shouldn't add all of these extra if statements in a core function for a non-semantic change.

facebook-github-bot pushed a commit that referenced this pull request May 15, 2020
Summary:
After an early return, we conditionalize all further execution. This means that currently the pattern of
`if return elif return elif return` generates better code than `if return if return if return`. It's obviously not good to have semantically equivalent code generate worse IR, so we should rewrite the graph to handle this case. This came up in #37171

```
torch.jit.script
def test_foo(x: bool, y: bool):
    if x:
        return 1
    return 2
print(test_foo.code)
```
generates:
```
def test_foo(x: bool,
    y: bool) -> int:
  _0 = uninitialized(int)
  if x:
    _1, _2 = True, 1
  else:
    _1, _2 = False, _0
  if _1:
    _3 = _2
  else:
    _3 = 2
  return _3
```
while
```
torch.jit.script
def test_foo(x: bool, y: bool):
    if x:
        return 1
    else:
        return 2
print(test_foo.code)
```
generates:
```
def test_foo(x: bool,
    y: bool) -> int:
  if x:
    _0 = 1
  else:
    _0 = 2
  return _0
```
Pull Request resolved: #38282

Differential Revision: D21576733

Pulled By: eellison

fbshipit-source-id: 80cf1ad7fbda6d8d58557abbfb21c90eafae7488
dreiss added 5 commits May 22, 2020 11:29
Every one of these branches returns or raises, so there's no need for elif.
This makes it a little easier to reorder and move conditions.

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992/)

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992)

[ghstack-poisoned]
Every one of these branches returns or raises, so there's no need for elif.
This makes it a little easier to reorder and move conditions.

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992/)

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992)

[ghstack-poisoned]
Every one of these branches returns or raises, so there's no need for elif.
This makes it a little easier to reorder and move conditions.

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992/)

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992)

[ghstack-poisoned]
Every one of these branches returns or raises, so there's no need for elif.
This makes it a little easier to reorder and move conditions.

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992/)

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992)

[ghstack-poisoned]
Every one of these branches returns or raises, so there's no need for elif.
This makes it a little easier to reorder and move conditions.

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992/)

Differential Revision: [D21209992](https://our.internmc.facebook.com/intern/diff/D21209992)

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 8f0e254.

@facebook-github-bot facebook-github-bot deleted the gh/dreiss/56/head branch July 11, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants