Skip to content

Index add new#3209

Merged
bdhirsh merged 5 commits into
masterfrom
index_add_new
Dec 15, 2021
Merged

Index add new#3209
bdhirsh merged 5 commits into
masterfrom
index_add_new

Conversation

@JackCaoG
Copy link
Copy Markdown
Collaborator

This is to fix the compilation error introduced by pytorch/pytorch#65993. Will support alpha properly in a separate pr.

Copy link
Copy Markdown
Contributor

@bdhirsh bdhirsh left a comment

Choose a reason for hiding this comment

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

not supporting the alpha param for now sounds good to me.

I think you'll actually need to change the index_add_ lowering to index_add though, now that it's a structured kernel in core. That's probably better anyway, since we prefer pytorch/xla's lowerings to be functional, right? I think the current state is just a byproduct of the fact that the old index_add kernel in core was composite. I see some test failures, but my guess of what's happening now is:

  • you call aten::index_add
  • XLA doesn't have a lowering: we hit a code-generated kernel that redispatches to aten::index_add.out
  • XLA also doesn't have a lowering for index_add.out so things break

@JackCaoG
Copy link
Copy Markdown
Collaborator Author

not supporting the alpha param for now sounds good to me.

I think you'll actually need to change the index_add_ lowering to index_add though, now that it's a structured kernel in core. That's probably better anyway, since we prefer pytorch/xla's lowerings to be functional, right? I think the current state is just a byproduct of the fact that the old index_add kernel in core was composite. I see some test failures, but my guess of what's happening now is:

  • you call aten::index_add
  • XLA doesn't have a lowering: we hit a code-generated kernel that redispatches to aten::index_add.out
  • XLA also doesn't have a lowering for index_add.out so things break

Good point, I will do that instead

@JackCaoG JackCaoG requested a review from bdhirsh November 15, 2021 18:24
Copy link
Copy Markdown
Contributor

@bdhirsh bdhirsh left a comment

Choose a reason for hiding this comment

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

LGTM. Didn't realize we already have an (unused?) index_add implementation lying around 😛

@bdhirsh
Copy link
Copy Markdown
Contributor

bdhirsh commented Dec 15, 2021

merging now that pytorch/pytorch#65993 has gone through

@bdhirsh bdhirsh merged commit 230f904 into master Dec 15, 2021
@bdhirsh bdhirsh deleted the index_add_new branch December 15, 2021 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants