[te] Don't fuse integer fmod or remainder by bertmaher · Pull Request #48700 · pytorch/pytorch

bertmaher · 2020-12-02T07:12:43Z

Stack from ghstack:

[te] Don't fuse integer fmod or remainder #48700 [te] Don't fuse integer fmod or remainder

fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code.

Differential Revision: D25265792

fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/) [ghstack-poisoned]

fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/) ghstack-source-id: 117634182 Pull Request resolved: #48700

ZolotukhinM

Overall looks good, but I wonder if division by 0 was the only issue. There is another test called test_binary_div_ops that doesn't try to divide by 0 but still fails with int8. Only fusing floats would solve that too, but I'm curious what was the problem there - we might have some other bug.

ZolotukhinM · 2020-12-02T07:20:56Z

+          if (!st || !isFloatingType(*st)) {
+            return false;
+          }
+        } else if (!v->type()->cast<FloatType>()) {


This doesn't include Half and Double, does it?

It might. We don't seem to differentiate statically halfs, doubles and single-points, @eellison ?

this check is on a non-Tensor type (Scalar value), so Half/Double doesn't apply here

ZolotukhinM · 2020-12-02T07:21:23Z

            return lambda x, y: fn(x, y)

        dtypes = [
-            # FIXME: Fails in IR Eval: torch.int8 and_ cpu


This FIXME is for a different issue.

What's the issue? It looked like a vestigial FIXME.

The issue is that the combination op=and, dtype=int8, device=cpu fails on IR eval :) I added it in one of the recent commits

Ohhh, so what threw me off was this being in the block of dtypes. I read it as saying the combination of int8 and cpu fails :-p. Anyways I'll put it back

bertmaher · 2020-12-02T08:11:20Z

Overall looks good, but I wonder if division by 0 was the only issue. There is another test called test_binary_div_ops that doesn't try to divide by 0 but still fails with int8. Only fusing floats would solve that too, but I'm curious what was the problem there - we might have some other bug.

Actually there are at least two other issues which end up being side-stepped by this PR. I should probably fix them too, really, although they become more annoying to test if ints are banned in fusion (which I still think is the right call for the ZeroDivisionError reason).

We translate fmod(int, float) to (int)fmod(float, float), but eager mode translates it to (int)(int % int). Banning ints at the fuser level masks this bad translation.
Even if we somehow did create TE for fmod(int, int) we throw unsupported_dtype from llvm codegen. Which isn't entirely wrong, since we arguably should translate it as normal mod(int, int).

dr-ci · 2020-12-02T08:16:17Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

Extra GitHub checks: 1 failed

Failed: GitHub Actions - clang-tidy

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.

See how this bot performed.

This comment has been revised 33 times.

eellison

LGTM

eellison · 2020-12-02T17:10:45Z

+          if (!st || !isFloatingType(*st)) {
+            return false;
+          }
+        } else if (!v->type()->cast<FloatType>()) {


this check is on a non-Tensor type (Scalar value), so Half/Double doesn't apply here

eellison · 2020-12-02T17:14:12Z

+          if (!st || !isFloatingType(*st)) {
+            return false;
+          }
+        } else if (!v->type()->cast<FloatType>()) {


You could also allow constant non-zero ints:

(v->type()->cast<IntType>() && constant_as<int>(v).value_or(0) != 0)

eellison · 2020-12-02T17:17:24Z

      }
    }

+    // Value is only supported if operands are floats.


Do we need to exclude non-floating point tensors if they are the first input ?

That's a good point, this could be relaxed.

fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/) [ghstack-poisoned]

Pull Request resolved: #48700 fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. ghstack-source-id: 117741914 Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/)

…der" fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/) [ghstack-poisoned]

fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/) [ghstack-poisoned]

Pull Request resolved: #48700 fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. ghstack-source-id: 117821429 Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/)

…mod or remainder" fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/) [ghstack-poisoned]

Pull Request resolved: #48700 fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. ghstack-source-id: 117845642 Differential Revision: [D25265792](https://our.internmc.facebook.com/intern/diff/D25265792/)

codecov · 2020-12-04T10:19:14Z

Codecov Report

Merging #48700 (2d267c9) into gh/bertmaher/36/base (90a3049) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                  Coverage Diff                  @@
##           gh/bertmaher/36/base   #48700   +/-   ##
=====================================================
  Coverage                 80.83%   80.83%           
=====================================================
  Files                      1860     1860           
  Lines                    200742   200751    +9     
=====================================================
+ Hits                     162274   162285   +11     
+ Misses                    38468    38466    -2

eellison · 2020-12-04T17:13:44Z

    return true;
  }

+  bool typesAreSupported(const Node* node) {


While you’re at it, could you move the cuda_only fuse set ?

facebook-github-bot · 2020-12-05T03:18:01Z

This pull request has been merged in 2d07d5b.

Summary: Pull Request resolved: pytorch#48700 fmod and remainder on int tensors will raise ZeroDivisionError if their divisors are 0. I don't think we should try to generate code that raises exceptions. If at some point we really wanted to fuse these, I might lean towards calling a C++ helper function from the generated code. ghstack-source-id: 117845642 Test Plan: `buck test //caffe2/test:jit -- test_binary_ops` Reviewed By: eellison Differential Revision: D25265792 fbshipit-source-id: 0be56ba3feafa1dbf3c37f6bb8c1550cb6891e6d

facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Dec 2, 2020

bertmaher requested review from Krovatkin, ZolotukhinM and eellison December 2, 2020 07:14

ZolotukhinM reviewed Dec 2, 2020

View reviewed changes

eellison approved these changes Dec 2, 2020

View reviewed changes

eellison reviewed Dec 4, 2020

View reviewed changes

facebook-github-bot closed this in 2d07d5b Dec 5, 2020

facebook-github-bot added the Merged label Dec 5, 2020

facebook-github-bot deleted the gh/bertmaher/36/head branch December 8, 2020 15:23

guilhermeleobas mentioned this pull request Dec 21, 2020

add type annotations to torch.nn.quantized.modules.conv #49702

Closed

Conversation

bertmaher commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZolotukhinM 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!

bertmaher commented Dec 2, 2020

Uh oh!

dr-ci Bot commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Extra GitHub checks: 1 failed

Uh oh!

eellison 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!

codecov Bot commented Dec 4, 2020

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bertmaher commented Dec 2, 2020 •

edited

Loading

dr-ci Bot commented Dec 2, 2020 •

edited

Loading