KEMBAR78
[jit] Add `bool` type to IR by driazati · Pull Request #11834 · pytorch/pytorch · GitHub
Skip to content

[jit] Add bool type to IR #11834

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 24 commits into from
Closed

[jit] Add bool type to IR #11834

wants to merge 24 commits into from

Conversation

@driazati
Copy link
Contributor

@driazati driazati commented Sep 18, 2018 •
edited
Loading

This PR adds a bool type to IValue and puts it into place.

  • changes conds for prim::If and prim::Loop to use bool type
  • changes operators that take bools to match their native ops
  • fixes ambiguous aten ops aten::std and aten::var
    • fixes tests in test_jit.py TestJitGenerated 
       'test_std_dim',	
 'test_std_dim_1d',	
 'test_std_dim_1d_neg0',	
 'test_std_dim_neg0',	
 'test_var_dim',	
 'test_var_dim_1d',	
 'test_var_dim_1d_neg0',	
 'test_var_dim_neg0'
  • adds prim::BoolToTensor and prim::TensorToBool

@apaszke @zdevito

@pytorchbot pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 18, 2018
facebook-github-bot
facebook-github-bot reviewed Sep 18, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito
zdevito reviewed Sep 19, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

This looks good -- I just have a bunch of small fixes.

test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/constants.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/type.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

David Riazati added 10 commits September 19, 2018 09:23
Add Bool type to IR
b10fedf 
Summary:
* Adds explicit bool type and removes previous coercion from bool to int
* A couple tests now pass as a result

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Rebase
518b48c 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Remove more tests that are now passing
2abbfbc 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Add prim::TensorToBool and emit bool in conds
d47c359 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Add bool support for prim::Loop
d56b7c9 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Expected test changes
06bf75e 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Rebase
60f67d1 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Update shape analysis
0de1cb5 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
More fixes
c320183 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Address comments + rebase
0c6e14b 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Fix typo
a99c68b 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@driazati
Copy link
Contributor Author

Should the bool type be considered a scalar backed by an int? Would manifest in IValue::isScalar and IValue::toScalar

Use standard types
a1bbe48 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
David Riazati added 2 commits September 19, 2018 16:11
Fix ivalue bug
5a0534f 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Merge branch 'master' of https://github.com/pytorch/pytorch into ir_bool
ae3abc8
@fmassa
Copy link
Member

fmassa commented Sep 25, 2018

@driazati this needs a rebase

David Riazati added 5 commits September 26, 2018 17:02
Merge
1f47e7a
Merge Pt. 2
3654723
Cleanup
90251f2 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Merge branch 'master' of https://github.com/pytorch/pytorch into ir_bool
55141a1
Update
06d795c 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
driazati
driazati commented Sep 27, 2018
View reviewed changes
test/expect/TestBatched.test_if_else_with_scalar.expect
%other : float = prim::TensorToNum(%7)
%9 : Dynamic = aten::gt(%a.1_data, %other)
%10 : int = prim::TensorToNum(%9)
%10 : bool = prim::TensorToBool(%9)

This comment was marked as off-topic.

Sign in to view

zdevito
zdevito approved these changes Sep 28, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

This looks good to go once rebased. In a followup we should add the remaining numeric<->bool conversions to match python behavior.

torch/csrc/jit/type.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/onnx/utils.py Outdated
elif n.kindOf("value") == "is":
value = torch.stack([torch.tensor(v) for v in n["value"]]) if n["value"] else []
return g.op("Constant", value_t=value)
elif n.kindOf("value") == "i":

This comment was marked as off-topic.

Sign in to view

David Riazati added 2 commits September 28, 2018 13:35
Remove bool parameter on fromBoolType
f51c828 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Merge
d35c54c
@driazati
Copy link
Contributor Author

driazati commented Oct 1, 2018

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed Oct 1, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Fix missing bool cases in erase_number_types
ecc9fc9 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
facebook-github-bot
facebook-github-bot reviewed Oct 1, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot
facebook-github-bot reviewed Oct 3, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

driazati is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Oct 3, 2018
@facebook-github-bot
Add bool type to IR (#11834)
874f71c 
Summary:
This PR adds a bool type to `IValue` and puts it into place.

* changes conds for `prim::If` and `prim::Loop` to use `bool` type
* changes operators that take `bool`s to match their native ops
* fixes ambiguous `aten` ops `aten::std` and `aten::var`
	* fixes tests in `test_jit.py TestJitGenerated`
		```
		'test_std_dim',
		'test_std_dim_1d',
		'test_std_dim_1d_neg0',
		'test_std_dim_neg0',
		'test_var_dim',
		'test_var_dim_1d',
		'test_var_dim_1d_neg0',
		'test_var_dim_neg0'
		```
* adds `prim::BoolToTensor` and `prim::TensorToBool`

apaszke zdevito
Pull Request resolved: pytorch/pytorch#11834

Differential Revision: D9928570

Pulled By: driazati

fbshipit-source-id: 373c53df2f1a8ffa9e33d9a517002fbeef25f3eb
@ezyang ezyang added the merged label Jun 26, 2019
@hukkelas hukkelas mentioned this pull request Jul 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zdevito zdevito zdevito approved these changes

@anderspapitto anderspapitto Awaiting requested review from anderspapitto

@bddppq bddppq Awaiting requested review from bddppq

@colesbury colesbury Awaiting requested review from colesbury

@dzhulgakov dzhulgakov Awaiting requested review from dzhulgakov

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@houseroad houseroad Awaiting requested review from houseroad

@jamesr66a jamesr66a Awaiting requested review from jamesr66a

@smessmer smessmer Awaiting requested review from smessmer

@soumith soumith Awaiting requested review from soumith

@Yangqing Yangqing Awaiting requested review from Yangqing

+1 more reviewer

@apaszke apaszke apaszke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@driazati @fmassa @zdevito @apaszke @facebook-github-bot @ezyang @pytorchbot