E2E SymInt example narrow_copy by Krovatkin · Pull Request #75759 · pytorch/pytorch

Krovatkin · 2022-04-13T20:37:04Z

This roughly corresponds to Goal 3.2 in https://docs.google.com/document/d/1iiLNwR5ohAsw_ymfnOpDsyF6L9RTUaHMpD8YLw-jxEw/edit#

Namely:

It adds the following:

SymbolicIntNode interface
LazySymbolicIntNode implementation
Lazy narrow_copy implementation
Need add support for SymInt in codegen
Test (below)

TEST(LazyDynamicOpsTest, NarrowCopy) {
  auto x = torch::rand({5, 10, 10}).to(kLazy);
  const size_t Y_DIM = 3;
  const size_t X_DIM_INDEX = 2;
  auto y = torch::rand({Y_DIM}).to(kLazy);
  auto ly = torch::lazy::TryGetLtcTensor(y);
  auto dim_node = MakeNode<SizeNode>(ly->GetIrValue(), 0);
  auto lmn = new torch::lazy::SymbolicIntNode(dim_node);
  auto z = x.narrow_copy(X_DIM_INDEX, 0, lmn->toSymInt());
  AllClose(z.cpu(), x.cpu().narrow_copy(X_DIM_INDEX, 0, Y_DIM));
}

facebook-github-bot · 2022-04-13T20:37:12Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/75759
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

💊 CI failures summary and remediations

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

Expand to see more

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

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

ezyang · 2022-04-17T03:41:06Z

I am confused. SymInt is the thing that fits in int64_t (and is where all the bitmasking logic ought to live). SymbolicIntNode is supposed to be the boxed thing that you can put all of the random extra IR bits you need to identify the object in question!

ezyang · 2022-04-17T03:46:41Z

We shouldn't expose data directly, since it isn't really an int64_t, it might also be some masked nonsense. Also, standard practice in C++ is when we're doing bit-masky stuff, we should represent it as a uint64_t, not a signed integer; signed integers are undefined with bitwise operators, see https://wiki.sei.cmu.edu/confluence/display/c/INT13-C.+Use+bitwise+operators+only+on+unsigned+operands

Sure, I can switch to uint64_t. I was trying to avoid conversions from int64_t to uint64_t since that's the type we are using for dimension values and we aren't doing any "dangerous" bit-wise stuff as shifting.

ezyang · 2022-04-17T03:47:12Z

instead of doing this make the vector store an owning pointer (either unique_ptr or shared_ptr) so that you don't need an explicit destructor

Yeah, I was thinking we could save some overhead here, but it may not worth it.

unique_ptr is supposed to be zero overhead on top of a regular raw pointer (due to ABI shenanigans this is not strictly true but you are mostly definitely not thinking about that here.)

ezyang · 2022-04-17T03:53:18Z

I wouldn't call the transformation you need to do here memoization; instead, the invariant is that SymbolicIntNodes should only ever be allocated SIMULTANEOUSLY with assigning them in the table, so they know their index in that case.

wconstab · 2022-04-18T21:12:54Z

So this tests that a symint can behave correctly in a static condition, and serves as a smoke test. But i guess it doesn't really cover that dynamic aspects?

yeah, though I could extend this and see how it behaves with different values, but this wasn't the goal of the milestone :)

wconstab · 2022-04-18T21:22:21Z

hm shouldn't you lock before you check?

wconstab · 2022-04-18T21:27:51Z

does 'or_list' mean list of symint in this case?

is this generated code correct for both cases? (maybe, as it is a base class)

yes, I'm thinking of reusing this flag for both symints and list of symints, we don't quite yet have lists of symints if it's confusing please let me know I'll rename the flag latter.

wconstab · 2022-04-18T21:30:18Z

i guess this works for all cases (where operand(0) is a constant, a list of constants, a Value, etc.?)

it's cool to see this E2E finally!

Err I think operand(0) should be a tensor otherwise JIT would burp.

miladm

Thanks @Krovatkin!

I added a couple of comments and questions.

miladm · 2022-04-18T20:59:54Z

Looks like data_ is a private member? Do we beed need sci.date() here?

we are inside SymInt this and sci is also SymInt. I could switch it to sci_.data() if you think it looks better.

miladm · 2022-04-18T23:43:30Z

Trying to locate how exactly lmn->toSymInt()is propagated to the Lower function. Would be great to discuss it in our meeting tomorrow.

Assuming the SymInt parameter allows a call to the upstream SizeNode object via the toSymbolicIntNode() API.

yeah this is exactly how it works! Hopefully, it's intuitive enough API, but we can work on it.

miladm · 2022-04-18T23:45:18Z

nit:
We use int64_t and uint64_t for data_. Is it necessary to let size_t propagate as well?

yeah I need to do a better job here to reduce implicit conversions

wconstab

lgtm pending addressing the rest of the review comments!

ezyang · 2022-04-23T18:03:58Z

for the future: could you avoid force pushing, it makes it hard for me to see what differentially changed in the revision. Since you're not using stack diffs it doesn't make a material difference if you rebase or merge, but merging preserves history.

Krovatkin · 2022-04-26T02:39:18Z

@pytorchbot merge this

github-actions · 2022-04-26T02:41:09Z

Hey @Krovatkin.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: This **roughly** corresponds to Goal 3.2 in https://docs.google.com/document/d/1iiLNwR5ohAsw_ymfnOpDsyF6L9RTUaHMpD8 (d843f63f2a49c339fe79c3016a082c60fbf3fed5)YLw-jxEw/edit Namely: It adds the following: * SymbolicIntNode interface * LazySymbolicIntNode implementation * Lazy `narrow_copy` implementation * Need add support for SymInt in codegen * Test (below) ```cpp TEST(LazyDynamicOpsTest, NarrowCopy) { auto x = torch::rand({5, 10, 10}).to(kLazy); const size_t Y_DIM = 3; const size_t X_DIM_INDEX = 2; auto y = torch::rand({Y_DIM}).to(kLazy); auto ly = torch::lazy::TryGetLtcTensor(y); auto dim_node = MakeNode<SizeNode>(ly->GetIrValue(), 0); auto lmn = new torch::lazy::SymbolicIntNode(dim_node); auto z = x.narrow_copy(X_DIM_INDEX, 0, lmn->toSymInt()); AllClose(z.cpu(), x.cpu().narrow_copy(X_DIM_INDEX, 0, Y_DIM)); } ``` Pull Request resolved: #75759 Approved by: https://github.com/wconstab Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/bb60cac25af1f5e616432350f0ff5eb600ddc5e8 Reviewed By: osalpekar Differential Revision: D35938189 Pulled By: Krovatkin fbshipit-source-id: 59e5371efd27a635b83b4311fa53bb0e11af332b

This **roughly** corresponds to Goal 3.2 in https://docs.google.com/document/d/1iiLNwR5ohAsw_ymfnOpDsyF6L9RTUaHMpD8YLw-jxEw/edit# Namely: It adds the following: * SymbolicIntNode interface * LazySymbolicIntNode implementation * Lazy `narrow_copy` implementation * Need add support for SymInt in codegen * Test (below) ```cpp TEST(LazyDynamicOpsTest, NarrowCopy) { auto x = torch::rand({5, 10, 10}).to(kLazy); const size_t Y_DIM = 3; const size_t X_DIM_INDEX = 2; auto y = torch::rand({Y_DIM}).to(kLazy); auto ly = torch::lazy::TryGetLtcTensor(y); auto dim_node = MakeNode<SizeNode>(ly->GetIrValue(), 0); auto lmn = new torch::lazy::SymbolicIntNode(dim_node); auto z = x.narrow_copy(X_DIM_INDEX, 0, lmn->toSymInt()); AllClose(z.cpu(), x.cpu().narrow_copy(X_DIM_INDEX, 0, Y_DIM)); } ``` Pull Request resolved: pytorch#75759 Approved by: https://github.com/wconstab

facebook-github-bot added the cla signed label Apr 13, 2022

Krovatkin changed the title ~~E2E SymInt example narrow_copy~~ E2E SymInt example narrow_copy [WIP] Apr 13, 2022

Krovatkin requested review from miladm and wconstab and removed request for wconstab April 13, 2022 20:38

Krovatkin changed the title ~~E2E SymInt example narrow_copy [WIP]~~ E2E SymInt example narrow_copy Apr 14, 2022

Krovatkin requested review from JackCaoG, ezyang, wconstab and zou3519 April 14, 2022 21:35

Krovatkin force-pushed the krovatkin/symbolic_int_node branch 2 times, most recently from 7e5bdfb to c5371ed Compare April 15, 2022 21:36

ezyang reviewed Apr 17, 2022

View reviewed changes

wconstab reviewed Apr 18, 2022

View reviewed changes

Comment thread tools/codegen/api/lazy.py Outdated

wconstab reviewed Apr 18, 2022

View reviewed changes

miladm reviewed Apr 18, 2022

View reviewed changes

Krovatkin force-pushed the krovatkin/symbolic_int_node branch from c5b0f50 to c83bf7e Compare April 20, 2022 17:31

Krovatkin requested a review from wconstab April 20, 2022 19:03

Krovatkin force-pushed the krovatkin/symbolic_int_node branch from c83bf7e to 3199a18 Compare April 20, 2022 19:11

wconstab approved these changes Apr 20, 2022

View reviewed changes

Krovatkin force-pushed the krovatkin/symbolic_int_node branch 2 times, most recently from fe08e31 to 794c08a Compare April 22, 2022 23:45

Krovatkin force-pushed the krovatkin/symbolic_int_node branch from 794c08a to 5129cf5 Compare April 24, 2022 17:43

Krovatkin added 3 commits April 25, 2022 10:26

narrowcopy test

68ee5c1

missed a file

2951681

move to Aten/core

8d2468f

Krovatkin force-pushed the krovatkin/symbolic_int_node branch from 0c85fce to 8d2468f Compare April 25, 2022 17:26

fix warnings

aca2fc8

pytorchmergebot closed this in bb60cac Apr 26, 2022

miladm mentioned this pull request May 10, 2022

[POC] Dynamic expand with SymInt implementation pytorch/xla#3558

Draft

13 tasks

Conversation

Krovatkin commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miladm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wconstab left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Apr 23, 2022

Uh oh!

Krovatkin commented Apr 26, 2022

Uh oh!

github-actions Bot commented Apr 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Krovatkin commented Apr 13, 2022 •

edited

Loading

facebook-github-bot commented Apr 13, 2022 •

edited

Loading