Skip to content

gcd: port to structured #57624

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
Closed
Freey0 wants to merge 2 commits into from
Closed

gcd: port to structured #57624

Freey0 wants to merge 2 commits into from

Conversation

@Freey0
Copy link
Contributor

@Freey0 Freey0 commented May 5, 2021 •
edited by ezyang
Loading

Stack from ghstack:

Differential Revision: D28224832

@Freey0
Port gcd to structured
b052f4a 
[ghstack-poisoned]
@Freey0 Freey0 requested a review from ezyang as a code owner May 5, 2021 14:10
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 5, 2021 •
edited
Loading

💊 CI failures summary and remediations

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

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-scanned failure(s)
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 to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

This was referenced May 5, 2021
Closed
Closed
Closed
Closed
Closed
Closed
@ezyang
Copy link
Contributor

ezyang commented May 5, 2021

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@Freey0
Copy link
Contributor Author

Freey0 commented May 6, 2021

A related question.
Functions like gcd, nextafter ... these functions only support some input types, and the current implementation throws unsupported errors via AT_DISPATCH_XX_TYPES in xx_stub.

void gcd_kernel(TensorIteratorBase& iter) {
  AT_DISPATCH_INTEGRAL_TYPES(iter.dtype(), "gcd_cpu", [&]() {
      cpu_kernel(
          iter,
          [](scalar_t a, scalar_t b) -> scalar_t {
            return calc_gcd(a, b);
          });
    });
}

#define AT_DISPATCH_INTEGRAL_TYPES(TYPE, NAME, ...)                         \
  [&] {                                                                     \
    const auto& the_type = TYPE;                                            \
    /* don't use TYPE again in case it is an expensive or side-effect op */ \
    at::ScalarType _st = ::detail::scalar_type(the_type);                   \
    RECORD_KERNEL_FUNCTION_DTYPE(NAME, _st);                                \
    switch (_st) {                                                          \
      AT_PRIVATE_CASE_TYPE(NAME, at::ScalarType::Byte, uint8_t, __VA_ARGS__)      \
      AT_PRIVATE_CASE_TYPE(NAME, at::ScalarType::Char, int8_t, __VA_ARGS__)       \
      AT_PRIVATE_CASE_TYPE(NAME, at::ScalarType::Int, int32_t, __VA_ARGS__)       \
      AT_PRIVATE_CASE_TYPE(NAME, at::ScalarType::Long, int64_t, __VA_ARGS__)      \
      AT_PRIVATE_CASE_TYPE(NAME, at::ScalarType::Short, int16_t, __VA_ARGS__)     \
      default:                                                              \
        AT_ERROR(#NAME, " not implemented for '", toString(_st), "'");      \
    }                                                                       \
  }()

The metafunctions ported this way do not check for correct input types, the CI can pass because the tests only test for supported types.

    @onlyOnCPUAndCUDA
    @dtypes(torch.uint8, torch.int8, torch.int16, torch.int32, torch.int64)
    def test_gcd(self, device, dtype):

Any ideas would be greatly appreciated! @ezyang

@ezyang
Copy link
Contributor

ezyang commented May 6, 2021

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang
Copy link
Contributor

ezyang commented May 6, 2021

Good question. A somewhat distasteful answer is to add "check" variants of AT_DISPATCH_INTEGRAL_TYPES which don't redispatch, but just check that the dtype is appropriate and raise the error if it is not supported. This is somewhat distasteful because (1) that's gonna to be a bunch of copy pasting in the macros and we have to keep them in sync, and (2) it's going to be slower than what we did before, since you're banging on dtype twice. I filed an issue about this at #57742

I don't have a big problem if you want to "hush hush" commit a bug here because the CI coverage is not quite enough. We can also just suppress these meta tests with an expected failure and file an issue to fix this later. If you want to do the right thing, I think adding check variants is the only easy thing to do, but we'll need to get some other feedback on it (cc @gchanan).

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 7, 2021

@ezyang merged this pull request in 7e51ac5.

@facebook-github-bot facebook-github-bot deleted the gh/Feey0/9/head branch May 11, 2021 14:16
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
@Freey0
Port gcd to structured (pytorch#57624)
913c83d 
Summary: Pull Request resolved: pytorch#57624

Test Plan: Imported from OSS

Reviewed By: VitalyFedyunin

Differential Revision: D28224832

Pulled By: ezyang

fbshipit-source-id: 30a8eba025c67d990103e49c03a396810f9d4006
@bdhirsh bdhirsh changed the title Port gcd to structured gcd: port to structured May 24, 2021
@Freey0 Freey0 mentioned this pull request Jun 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

Assignees

No one assigned

Labels

cla signed Merged open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Freey0 @facebook-github-bot @ezyang @pytorchbot