Skip to content

[jit] Add bool type to IR#11834

Closed
driazati wants to merge 24 commits intopytorch:masterfrom
driazati:ir_bool
Closed

[jit] Add bool type to IR#11834
driazati wants to merge 24 commits intopytorch:masterfrom
driazati:ir_bool

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 11 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

driazati commented Sep 19, 2018

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

David Riazati added 2 commits September 19, 2018 12:43
Use standard types
a1bbe48 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Fix ivalue bug
5a0534f 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@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