[JIT][Reland] add list() support #42382
Conversation
facebook-github-bot
added
the
oncall: jit
💊 CI failures summary and remediationsAs of commit 213fcc5 (more details on the Dr. CI page):
Extra GitHub checks: 1 failed
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. This comment has been revised 17 times. |
eellison
requested review from
gmagogsfm,
SplitInfinity and
ansley
and removed request for
gmagogsfm,
SplitInfinity and
ansley
| @@ -2896,6 +2896,39 @@ struct to_ir { | |||
| } | |||
| return iterable_tree; | |||
| } | |||
| case prim::list: { | |||
| if (apply.inputs().size() == 0) { | |||
| TypePtr type = type_hint ? type_hint : ListType::ofTensors(); | |||
There was a problem hiding this comment.
ListTypePtr ListType::ofTensors() {
static auto value = ListType::create(TensorType::get());
return value;
}
Maybe ListType::create(TensorType::getInferred()) should be used (or a function that returns this) in this case since the type is being inferred.
There was a problem hiding this comment.
Agreed this would be nice, but imo it's a little awkward to have List[Inferred Tensor]. We're inferring the list itself. We do this other places like x = [] or x = {}, and would be nice to add as a follow up, but we dont have any inferred types on non-Tensor rn.
| @@ -130,6 +130,25 @@ def fn(x): | |||
| del x[1:3] | |||
| return x | |||
|
|
|||
| def test_list_keyword(self): | |||
| def foo(): | |||
| return list([1, 2, 3]), list(("a", "b")), list(range(5)), list("abcdefg") # noqa: C410 | |||
There was a problem hiding this comment.
For my own curiosity, how does the type for this expression get set correctly in the absence of an explicit annotation? The IR generation code that was added in this PR for list uses a given type_hint and assume List[Tensor] if there is none:
TypePtr type = type_hint ? type_hint : ListType::ofTensors();
There was a problem hiding this comment.
It's inferred from the input iterable when we emit the list comprehension
| } | ||
| } | ||
| const std::string& iter_name = createTempName("$_iter"); | ||
| environment_stack->setSugaredVar( |
There was a problem hiding this comment.
Should this be removed from the environment stack after emitting the list comprehension?
There was a problem hiding this comment.
It's not really necessary, createTempName creates a unique identifier which won't ever be used again, and $_iter is not user-valid variable name. In the other instances of createTempName we do not subsequently remove the temporary variable binding
There was a problem hiding this comment.
@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Codecov Report
@@ Coverage Diff @@
## master #42382 +/- ##
==========================================
+ Coverage 81.26% 81.46% +0.20%
==========================================
Files 1792 1792
Lines 186194 186216 +22
==========================================
+ Hits 151303 151695 +392
+ Misses 34891 34521 -370 |
Fixes #40869
Resubmit of #33818.
Adds support for
list()by desugaring it to a list comprehension.Last time I landed this it made one of the tests slow, and got unlanded. I think that's bc the previous PR changed the emission of
list()on a list input or a str input to a list comprehension, which is the more general way of emittinglist(), but also a little bit slower. I updated this version to emit to the builtin operators for these two case. Hopefully it can land without being reverted this time...