Skip to content

Update op argmin and argmax from manual lowering to codegen. #5586

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
Merged
merged 2 commits into from
Sep 18, 2023
Merged

Update op argmin and argmax from manual lowering to codegen. #5586

merged 2 commits into from
Sep 18, 2023

Conversation

@zpcore
Copy link
Member

@zpcore zpcore commented Sep 15, 2023

resolves #5583

Codegen for op argmin and argmax.

@zpcore zpcore added the codegen label Sep 15, 2023
lsy323
lsy323 approved these changes Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@lsy323 lsy323 left a comment

Choose a reason for hiding this comment

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

LGTM

@lsy323 lsy323 merged commit a008de3 into master Sep 18, 2023
@JackCaoG
Copy link
Collaborator

JackCaoG commented Sep 18, 2023

Shouldn't we also remove

xla/torch_xla/csrc/ops/arg_min.cpp

Line 14 in a008de3

return BuildArgMin(operands[0], dim, keepdim);
and corresponding .h and arg_max files?

@lsy323
Copy link
Collaborator

lsy323 commented Sep 18, 2023

@JackCaoG Good catch, yes, we need to remove arg_min/max.h/cpp. @zpcore Let's open a follow up PR to remove those unused files.

@lsy323
Copy link
Collaborator

lsy323 commented Sep 18, 2023 •
edited
Loading

Also maybe paste the generated code in the description for reference like this PR #4449

@zpcore
Copy link
Member Author

zpcore commented Sep 18, 2023 •
edited
Loading

Thanks, created a new pull request here: #5605.

@lsy323 lsy323 mentioned this pull request Sep 18, 2023
Merged
@miladm
Copy link
Collaborator

miladm commented Jan 18, 2024

Looks like this PR contributed to #3560. @wonjoolee95 are you maintaining the status of codegen ops on this issue?

cc @zpcore @bhavya01

@wonjoo-wj wonjoo-wj mentioned this pull request Jan 18, 2024
Open
@wonjoo-wj
Copy link
Collaborator

wonjoo-wj commented Jan 18, 2024

Looks like this PR contributed to #3560. @wonjoolee95 are you maintaining the status of codegen ops on this issue?

cc @zpcore @bhavya01

Yep, thanks for bringing this up. Just updated #3560 to include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lsy323 lsy323 lsy323 approved these changes

@JackCaoG JackCaoG Awaiting requested review from JackCaoG

@wonjoo-wj wonjoo-wj Awaiting requested review from wonjoo-wj

Assignees

@zpcore zpcore

Labels

codegen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Codegen argmin and argmax op

6 participants

@zpcore @JackCaoG @lsy323 @miladm @wonjoo-wj